Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prow pubsub: supports presubmit jobs #22777

Merged
merged 2 commits into from
Jul 12, 2021
Merged

Prow pubsub: supports presubmit jobs #22777

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41431a9
Prow pubsub: supports presubmit jobs
chaodaiG Jul 2, 2021
a1ce8f6
prow sub component initialize git client
chaodaiG Jul 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
prow sub component initialize git client
  • Loading branch information
@chaodaiG
chaodaiG committed Jul 12, 2021
commit a1ce8f694caf9d589357c55a6c5b9c6c93f346b0
1 change: 1 addition & 0 deletions prow/cmd/sub/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ go_library(
"//prow/crier/reporters/pubsub:go_default_library",
"//prow/flagutil:go_default_library",
"//prow/flagutil/config:go_default_library",
"//prow/git/v2:go_default_library",
"//prow/interrupts:go_default_library",
"//prow/logrusutil:go_default_library",
"//prow/metrics:go_default_library",
Expand Down
24 changes: 17 additions & 7 deletions prow/cmd/sub/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"k8s.io/test-infra/prow/crier/reporters/pubsub"
"k8s.io/test-infra/prow/flagutil"
configflagutil "k8s.io/test-infra/prow/flagutil/config"
"k8s.io/test-infra/prow/git/v2"
"k8s.io/test-infra/prow/interrupts"
"k8s.io/test-infra/prow/logrusutil"
"k8s.io/test-infra/prow/metrics"
Expand All @@ -45,6 +46,7 @@ var (

type options struct {
client flagutil.KubernetesOptions
github flagutil.GitHubOptions
port int
pushSecretFile string

Expand Down Expand Up @@ -80,6 +82,7 @@ func init() {

flagOptions.config.AddFlags(fs)
flagOptions.client.AddFlags(fs)
flagOptions.github.AddFlags(fs)
flagOptions.instrumentationOptions.AddFlags(fs)

fs.Parse(os.Args[1:])
Expand All @@ -93,16 +96,22 @@ func main() {
logrus.WithError(err).Fatal("Error starting config agent.")
}

var tokenGenerator func() []byte
var tokens []string
if flagOptions.pushSecretFile != "" {
var tokens []string
tokens = append(tokens, flagOptions.pushSecretFile)
}
if flagOptions.github.TokenPath != "" {
tokens = append(tokens, flagOptions.github.TokenPath)
}
secretAgent := &secret.Agent{}
if err := secretAgent.Start(tokens); err != nil {
logrus.WithError(err).Fatal("Error starting secrets agent.")
}
tokenGenerator := secretAgent.GetTokenGenerator(flagOptions.pushSecretFile)

secretAgent := &secret.Agent{}
if err := secretAgent.Start(tokens); err != nil {
logrus.WithError(err).Fatal("Error starting secrets agent.")
}
tokenGenerator = secretAgent.GetTokenGenerator(flagOptions.pushSecretFile)
gitClient, err := flagOptions.github.GitClient(secretAgent, flagOptions.dryRun)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this couple us to GitHub or does it work with Gerrit as well? We just need a git client so we should probably avoid coupling to GH.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This client is for inrepoconfig, which is not supported in gerrit yet. We can change here once inrepoconfig is supported in gerrit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, but I'm more wondering how this behaves without GitHub credentials supplied. In subscriber.go@198 we try to use the git client independent of whether we are using GitHub or Gerrit. It seems strange to use a client for a different code review provider when know it won't work and that also seems like it will result in misleading error messages.

Looks like we never try to use the git client until after we've checked if InRepoConfig is enabled for the repo and flag validation should pass if no GitHub flags are supplied so I think this is ok.
/shrug
/hold cancel

if err != nil {
logrus.WithError(err).Fatal("Error getting Git client.")
}

prowjobClient, err := flagOptions.client.ProwJobClient(configAgent.Config().ProwJobNamespace, flagOptions.dryRun)
Expand All @@ -125,6 +134,7 @@ func main() {
ConfigAgent: configAgent,
Metrics: promMetrics,
ProwJobClient: kubeClient,
GitClient: git.ClientFactoryFrom(gitClient),
Reporter: pubsub.NewReporter(configAgent.Config), // reuse crier reporter
}

Expand Down
4 changes: 3 additions & 1 deletion prow/pubsub/subscriber/subscriber.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ type Subscriber struct {
ConfigAgent *config.Agent
Metrics *Metrics
ProwJobClient ProwJobClient
GitClient git.ClientFactory
Reporter reportClient
}

Expand Down Expand Up @@ -187,6 +188,7 @@ func (prh *presubmitJobHandler) getProwJobSpec(cfg prowCfgClient, pe ProwJobEven
}
var headSHAGetters []func() (string, error)
for _, pull := range refs.Pulls {
pull := pull
headSHAGetters = append(headSHAGetters, func() (string, error) {
return pull.SHA, nil
chaodaiG marked this conversation as resolved.
Show resolved Hide resolved
})
Expand Down Expand Up @@ -253,7 +255,7 @@ func (s *Subscriber) handleMessage(msg messageInterface, subscription string) er
case periodicProwJobEvent:
jh = &periodicJobHandler{}
case presubmitProwJobEvent:
jh = &presubmitJobHandler{}
jh = &presubmitJobHandler{GitClient: s.GitClient}
case postsubmitProwJobEvent:
jh = &postsubmitJobHandler{}
default:
Expand Down