Improve flag processing

label_sync/main.go

@@ -38,6 +38,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/yaml"
 
+	"k8s.io/test-infra/prow/config/secret"
 	"k8s.io/test-infra/prow/flagutil"
 	"k8s.io/test-infra/prow/github"
 	"k8s.io/test-infra/prow/logrusutil"
@@ -131,7 +132,7 @@ type options struct {
 	github          flagutil.GitHubOptions
 }
 
-func gatherOptions() options {
+func gatherOptions() (opts options, deprecatedOptions bool) {
 	o := options{}
 	fs := flag.NewFlagSet(os.Args[0], flag.ExitOnError)
 	fs.BoolVar(&o.debug, "debug", false, "Turn on debug to be more verbose")
@@ -153,7 +154,31 @@ func gatherOptions() options {
 	fs.IntVar(&o.tokenBurst, "token-burst", defaultBurst, "Allow consuming a subset of hourly tokens in a short burst. DEPRECATED: use --github-allowed-burst")
 	o.github.AddCustomizedFlags(fs, flagutil.ThrottlerDefaults(defaultTokens, defaultBurst))
 	fs.Parse(os.Args[1:])
-	return o
+
+	deprecatedGitHubOptions := false
+	newGitHubOptions := false
+	fs.Visit(func(f *flag.Flag) {
+		switch f.Name {
+		case "github-endpoint",
+			"github-graphql-endpoint",
+			"github-token-path",
+			"github-hourly-tokens",
+			"github-allowed-burst":
+			newGitHubOptions = true
+		case "token",
+			"endpoint",
+			"graphql-endpoint",
+			"tokens",
+			"token-burst":
+			deprecatedGitHubOptions = true
+		}
+	})
+
+	if deprecatedGitHubOptions && newGitHubOptions {
+		logrus.Fatalf("deprecated GitHub options, include --endpoint, --graphql-endpoint, --token, --tokens, --token-burst cannot be combined with new --github-XXX counterparts")
+	}
+
+	return o, deprecatedGitHubOptions
 }
 
 func pathExists(path string) bool {
@@ -682,6 +707,28 @@ type client interface {
 	GetRepoLabels(string, string) ([]github.Label, error)
 }
 
+func newClient(tokenPath string, tokens, tokenBurst int, dryRun bool, graphqlEndpoint string, hosts ...string) (client, error) {
+	if tokenPath == "" {
+		return nil, errors.New("--token unset")
+	}
+
+	if err := secret.Add(tokenPath); err != nil {
+		logrus.WithError(err).Fatal("Error starting secrets agent.")
+	}
+
+	if dryRun {
+		return github.NewDryRunClient(secret.GetTokenGenerator(tokenPath), secret.Censor, graphqlEndpoint, hosts...), nil
+	}
+	c := github.NewClient(secret.GetTokenGenerator(tokenPath), secret.Censor, graphqlEndpoint, hosts...)
+	if tokens > 0 && tokenBurst >= tokens {
+		return nil, fmt.Errorf("--tokens=%d must exceed --token-burst=%d", tokens, tokenBurst)
+	}
+	if tokens > 0 {
+		c.Throttle(tokens, tokenBurst) // 300 hourly tokens, bursts of 100
+	}
+	return c, nil
+}
+
 // Main function
 // Typical run with production configuration should require no parameters
 // It expects:
@@ -692,7 +739,7 @@ type client interface {
 // Next run takes about 22 seconds to check if all labels are correct on all repos
 func main() {
 	logrusutil.ComponentInit()
-	o := gatherOptions()
+	o, deprecated := gatherOptions()
 
 	if o.debug {
 		logrus.SetLevel(logrus.DebugLevel)
@@ -721,21 +768,13 @@ func main() {
 			logrus.WithError(err).Fatalf("failed to write css file using css-template %s to css-output %s", o.cssTemplate, o.cssOutput)
 		}
 	case o.action == "sync":
-		deprecatedGitHubOptions := len(o.token) != 0 || o.endpoint.String() != github.DefaultAPIEndpoint || o.graphqlEndpoint != github.DefaultGraphQLEndpoint || o.tokens != defaultTokens || o.tokenBurst != defaultBurst
-		newGitHubOptions := len(o.github.TokenPath) != 0 || o.github.Endpoint.String() != github.DefaultAPIEndpoint || o.github.GraphqlEndpoint != github.DefaultGraphQLEndpoint || o.github.ThrottleHourlyTokens != defaultTokens || o.github.ThrottleAllowBurst != defaultBurst
-
-		if deprecatedGitHubOptions && newGitHubOptions {
-			logrus.Fatalf("deprecated GitHub options, include --endpoint, --graphql-endpoint, --token, --tokens, --token-burst cannot be combined with new --github-XXX counterparts")
-		}
-
-		if deprecatedGitHubOptions {
-			o.github.TokenPath = o.token
-			o.github.ThrottleHourlyTokens = o.tokens
-			o.github.ThrottleAllowBurst = o.tokenBurst
-			o.github.Endpoint = o.endpoint
-			o.github.GraphqlEndpoint = o.graphqlEndpoint
+		var githubClient client
+		var err error
+		if deprecated {
+			githubClient, err = newClient(o.token, o.tokens, o.tokenBurst, !o.confirm, o.graphqlEndpoint, o.endpoint.Strings()...)
+		} else {
+			githubClient, err = o.github.GitHubClient(!o.confirm)
 		}
-		githubClient, err := o.github.GitHubClient(!o.confirm)
 
 		if err != nil {
 			logrus.WithError(err).Fatal("failed to create client")

prow/flagutil/github.go

@@ -41,8 +41,8 @@ import (
 // Set AllowDirectAccess to be true if you want to suppress warnings on direct github access (without ghproxy).
 type GitHubOptions struct {
 	Host              string
-	Endpoint          Strings
-	GraphqlEndpoint   string
+	endpoint          Strings
+	graphqlEndpoint   string
 	TokenPath         string
 	AllowAnonymous    bool
 	AllowDirectAccess bool
@@ -118,8 +118,8 @@ func (o *GitHubOptions) addFlags(fs *flag.FlagSet, paramFuncs ...FlagParameter)
 	params := flagParams{
 		defaults: GitHubOptions{
 			Host:            github.DefaultHost,
-			Endpoint:        NewStrings(github.DefaultAPIEndpoint),
-			GraphqlEndpoint: github.DefaultGraphQLEndpoint,
+			endpoint:        NewStrings(github.DefaultAPIEndpoint),
+			graphqlEndpoint: github.DefaultGraphQLEndpoint,
 		},
 	}
 
@@ -129,9 +129,9 @@ func (o *GitHubOptions) addFlags(fs *flag.FlagSet, paramFuncs ...FlagParameter)
 
 	defaults := params.defaults
 	fs.StringVar(&o.Host, "github-host", defaults.Host, "GitHub's default host (may differ for enterprise)")
-	o.Endpoint = NewStrings(defaults.Endpoint.Strings()...)
-	fs.Var(&o.Endpoint, "github-endpoint", "GitHub's API endpoint (may differ for enterprise).")
-	fs.StringVar(&o.GraphqlEndpoint, "github-graphql-endpoint", defaults.GraphqlEndpoint, "GitHub GraphQL API endpoint (may differ for enterprise).")
+	o.endpoint = NewStrings(defaults.endpoint.Strings()...)
+	fs.Var(&o.endpoint, "github-endpoint", "GitHub's API endpoint (may differ for enterprise).")
+	fs.StringVar(&o.graphqlEndpoint, "github-graphql-endpoint", defaults.graphqlEndpoint, "GitHub GraphQL API endpoint (may differ for enterprise).")
 	fs.StringVar(&o.TokenPath, "github-token-path", defaults.TokenPath, "Path to the file containing the GitHub OAuth secret.")
 	fs.StringVar(&o.AppID, "github-app-id", defaults.AppID, "ID of the GitHub app. If set, requires --github-app-private-key-path to be set and --github-token-path to be unset.")
 	fs.StringVar(&o.AppPrivateKeyPath, "github-app-private-key-path", defaults.AppPrivateKeyPath, "Path to the private key of the github app. If set, requires --github-app-id to bet set and --github-token-path to be unset")
@@ -202,7 +202,7 @@ func (o *GitHubOptions) parseOrgThrottlers() error {
 // Validate validates GitHub options. Note that validate updates the GitHubOptions
 // to add default values for TokenPath and graphqlEndpoint.
 func (o *GitHubOptions) Validate(bool) error {
-	endpoints := o.Endpoint.Strings()
+	endpoints := o.endpoint.Strings()
 	for i, uri := range endpoints {
 		if uri == "" {
 			endpoints[i] = github.DefaultAPIEndpoint
@@ -222,10 +222,10 @@ func (o *GitHubOptions) Validate(bool) error {
 		logrus.Warn("It doesn't look like you are using ghproxy to cache API calls to GitHub! This has become a required component of Prow and other components will soon be allowed to add features that may rapidly consume API ratelimit without caching. Starting May 1, 2020 use Prow components without ghproxy at your own risk! https://github.com/kubernetes/test-infra/tree/master/ghproxy#ghproxy")
 	}
 
-	if o.GraphqlEndpoint == "" {
-		o.GraphqlEndpoint = github.DefaultGraphQLEndpoint
-	} else if _, err := url.Parse(o.GraphqlEndpoint); err != nil {
-		return fmt.Errorf("invalid -github-graphql-endpoint URI: %q", o.GraphqlEndpoint)
+	if o.graphqlEndpoint == "" {
+		o.graphqlEndpoint = github.DefaultGraphQLEndpoint
+	} else if _, err := url.Parse(o.graphqlEndpoint); err != nil {
+		return fmt.Errorf("invalid -github-graphql-endpoint URI: %q", o.graphqlEndpoint)
 	}
 
 	if (o.ThrottleHourlyTokens > 0) != (o.ThrottleAllowBurst > 0) {
@@ -308,8 +308,8 @@ func (o *GitHubOptions) baseClientOptions() github.ClientOptions {
 	return github.ClientOptions{
 		Censor:          secret.Censor,
 		AppID:           o.AppID,
-		GraphqlEndpoint: o.GraphqlEndpoint,
-		Bases:           o.Endpoint.Strings(),
+		GraphqlEndpoint: o.graphqlEndpoint,
+		Bases:           o.endpoint.Strings(),
 		MaxRequestTime:  o.maxRequestTime,
 		InitialDelay:    o.initialDelay,
 		MaxSleepTime:    o.maxSleepTime,

prow/flagutil/github_test.go

@@ -45,15 +45,15 @@ func TestGitHubOptions_Validate(t *testing.T) {
 		{
 			name: "when empty endpoint, sets graphql endpoint",
 			in: &GitHubOptions{
-				Endpoint: NewStrings(""),
+				endpoint: NewStrings(""),
 			},
 			expectedGraphqlEndpoint: github.DefaultGraphQLEndpoint,
 			expectedErr:             false,
 		},
 		{
 			name: "when invalid github endpoint, returns error",
 			in: &GitHubOptions{
-				Endpoint: NewStrings("not a github url"),
+				endpoint: NewStrings("not a github url"),
 			},
 			expectedErr: true,
 		},
@@ -109,7 +109,7 @@ func TestGitHubOptions_Validate(t *testing.T) {
 			if !testCase.expectedErr && err != nil {
 				t.Errorf("%s: expected no error but got one: %v", testCase.name, err)
 			}
-			if testCase.expectedGraphqlEndpoint != testCase.in.GraphqlEndpoint {
+			if testCase.expectedGraphqlEndpoint != testCase.in.graphqlEndpoint {
 				t.Errorf("%s: unexpected graphql endpoint", testCase.name)
 			}
 		})