-
Notifications
You must be signed in to change notification settings - Fork 66
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.
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
Add certification-component-id flag #1222
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,10 @@ import ( | |
"github.com/spf13/pflag" | ||
) | ||
|
||
var submit bool | ||
var ( | ||
submit bool | ||
componentID string | ||
) | ||
|
||
// runPreflight is introduced to make testing of this command possible, it has the same method signature as cli.RunPreflight. | ||
type runPreflight func(context.Context, func(ctx context.Context) (certification.Results, error), cli.CheckConfig, formatters.ResponseFormatter, lib.ResultWriter, lib.ResultSubmitter) error | ||
|
@@ -82,9 +85,21 @@ func checkContainerCmd(runpreflight runPreflight) *cobra.Command { | |
flags.String("pyxis-env", check.DefaultPyxisEnv, "Env to use for Pyxis submissions.") | ||
_ = viper.BindPFlag("pyxis_env", flags.Lookup("pyxis-env")) | ||
|
||
flags.String("certification-project-id", "", fmt.Sprintf("Certification Project ID from connect.redhat.com/projects/{certification-project-id}/overview\n"+ | ||
flags.String("certification-project-id", "", fmt.Sprintf("Certification project ID from connect.redhat.com/projects/{certification-project-id}/overview\n"+ | ||
"URL paramater. This value may differ from the PID on the overview page. (env: PFLT_CERTIFICATION_PROJECT_ID)")) | ||
_ = viper.BindPFlag("certification_project_id", flags.Lookup("certification-project-id")) | ||
_ = flags.MarkDeprecated("certification-project-id", "please use --certification-component-id instead") | ||
|
||
// Use a bound package-level var here. We are going to leave the rest of the code | ||
// using the Viper id of 'certification_project_id' in order to minimize changes | ||
// in the overall code base. | ||
// When --certification-project-id is fully removed, this should become bound in Viper | ||
flags.StringVar(&componentID, "certification-component-id", "", fmt.Sprintf("Certification component ID from connect.redhat.com/component/view/{certification-component-id}/images\n"+ | ||
"URL paramater. This value may differ from the component PID on the overview page. (env: PFLT_CERTIFICATION_COMPONENT_ID)")) | ||
// Here, we are forcing an env binding, so we can check that later. This should also | ||
// be moved to "automatic" once the old project id is removed | ||
_ = viper.BindEnv("certification_component_id") | ||
checkContainerCmd.MarkFlagsMutuallyExclusive("certification-project-id", "certification-component-id") | ||
|
||
flags.String("platform", rt.GOARCH, "Architecture of image to pull. Defaults to runtime platform.") | ||
_ = viper.BindPFlag("platform", flags.Lookup("platform")) | ||
|
@@ -210,28 +225,41 @@ func checkContainerPositionalArgs(cmd *cobra.Command, args []string) error { | |
} | ||
}) | ||
|
||
// --submit was specified | ||
viper := viper.Instance() | ||
|
||
// If the new flag is set, use that | ||
if cmd.Flag("certification-component-id").Changed { | ||
cmd.Flag("certification-project-id").Changed = true | ||
cmd.Flag("certification-component-id").Changed = false | ||
viper.Set("certification_project_id", componentID) | ||
} | ||
Comment on lines
+230
to
+235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm understanding, this is just "promotion" of the value from one flag to the other, right? And updating their Changed state is required to avoid MutualExclusion from throwing an error? Semantic & Nit: it reads weird to me that we're feeding the value of the new flag into the old flag. Disregard at will. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is awkward, but this makes it so that we don't have to go change a bunch of other places that use the project-id verbage. It does seem very awkward. It occurs to me now to that each of these places that are changed should be marked with a TODO so that they are easier to find later. |
||
|
||
// However, if the new env var is set, that's the priority | ||
if viper.IsSet("certification_component_id") { | ||
viper.Set("certification_project_id", viper.GetString("certification_component_id")) | ||
} | ||
Comment on lines
+237
to
+240
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this only necessary because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. |
||
|
||
// --submit was specified | ||
if submit { | ||
// If the flag is not marked as changed AND viper hasn't gotten it from environment, it's an error | ||
if !cmd.Flag("certification-project-id").Changed && !viper.IsSet("certification_project_id") { | ||
return fmt.Errorf("certification Project ID must be specified when --submit is present") | ||
return fmt.Errorf("certification component ID must be specified when --submit is present") | ||
} | ||
if !cmd.Flag("pyxis-api-token").Changed && !viper.IsSet("pyxis_api_token") { | ||
return fmt.Errorf("pyxis API Token must be specified when --submit is present") | ||
} | ||
|
||
// If the flag is marked as changed AND it's still empty, it's an error | ||
if cmd.Flag("certification-project-id").Changed && viper.GetString("certification_project_id") == "" { | ||
return fmt.Errorf("certification Project ID cannot be empty when --submit is present") | ||
return fmt.Errorf("certification component ID cannot be empty when --submit is present") | ||
} | ||
if cmd.Flag("pyxis-api-token").Changed && viper.GetString("pyxis_api_token") == "" { | ||
return fmt.Errorf("pyxis API Token cannot be empty when --submit is present") | ||
} | ||
|
||
// Finally, if either certification project id or pyxis api token start with '--', it's an error | ||
if strings.HasPrefix(viper.GetString("pyxis_api_token"), "--") || strings.HasPrefix(viper.GetString("certification_project_id"), "--") { | ||
return fmt.Errorf("pyxis API token and certification ID are required when --submit is present") | ||
return fmt.Errorf("pyxis API token and certification component ID are required when --submit is present") | ||
} | ||
} | ||
|
||
|
@@ -242,13 +270,15 @@ func checkContainerPositionalArgs(cmd *cobra.Command, args []string) error { | |
// and throws an error if the value provided is in a legacy format that is not usable to query pyxis | ||
func validateCertificationProjectID(cmd *cobra.Command, args []string) error { | ||
viper := viper.Instance() | ||
|
||
// From here on out, we just treat project ID like we did before. | ||
certificationProjectID := viper.GetString("certification_project_id") | ||
// splitting the certification project id into parts. if there are more than 2 elements in the array, | ||
// we know they inputted a legacy project id, which can not be used to query pyxis | ||
parts := strings.Split(certificationProjectID, "-") | ||
|
||
if len(parts) > 2 { | ||
return fmt.Errorf("certification project id: %s is improperly formatted see help command for instructions on obtaining proper value", certificationProjectID) | ||
return fmt.Errorf("certification component id: %s is improperly formatted see help command for instructions on obtaining proper value", certificationProjectID) | ||
} | ||
|
||
if parts[0] == "ospid" { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
pyxis_api_token: mytoken | ||
certification_component_id: mycomponentcertid |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
pyxis_api_token: mytoken | ||
certification_project_id: myprojectid |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -125,11 +125,24 @@ func WithDockerConfigJSONFromFile(s string) Option { | |||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// DEPRECATED: Use WithCertificationProject instead | ||||||||||||||||||||||||||||
// WithCertificationProject adds the project's id and pyxis token to the check | ||||||||||||||||||||||||||||
// allowing for the project's metadata to change the certification (if necessary). | ||||||||||||||||||||||||||||
// An example might be the Scratch or Privileged flags on a project allowing for | ||||||||||||||||||||||||||||
// the corresponding policy to be executed. | ||||||||||||||||||||||||||||
func WithCertificationProject(id, token string) Option { | ||||||||||||||||||||||||||||
Comment on lines
+128
to
133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Writing Deprecated (over DEPRECATED) seems to correctly trigger staticcheck when linting. Also, placing it at the end (or, potentially just separated by one blank comment link) makes staticcheck report the suggestion correctly. I wouldn't think it matters, but Godoc specifies the Title Case here. Also note that the statement is saying to use the same function it's deprecating. Should read "use WithCertificationComponent instead". Finally, I see two places where this is called in the project that need updating.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good suggestion. I'll implement that. |
||||||||||||||||||||||||||||
return withCertificationProject(id, token) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// WithCertificationComponent adds the component's id and pyxis token to the check | ||||||||||||||||||||||||||||
// allowing for the copmonent's metadata to change the certification (if necessary). | ||||||||||||||||||||||||||||
// An example might be the Scratch or Privileged flags on a project allowing for | ||||||||||||||||||||||||||||
// the corresponding policy to be executed. | ||||||||||||||||||||||||||||
func WithCertificationComponent(id, token string) Option { | ||||||||||||||||||||||||||||
return withCertificationProject(id, token) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
func withCertificationProject(id, token string) Option { | ||||||||||||||||||||||||||||
return func(cc *containerCheck) { | ||||||||||||||||||||||||||||
cc.pyxisToken = token | ||||||||||||||||||||||||||||
cc.certificationProjectID = id | ||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. Why not just bind this to viper now? It'll be bound at a different endpoint anyway, and you can resolve the two in the places you need, such as in runtime.NewConfigFrom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try a bunch of different permutations of all this, and this seemed to be the path of least resistance. It's been a bit since I looked at it, so I can't give you a full explanation right now. :)