Add certification-component-id flag #1222
Conversation
openshift-ci
bot
added
the
approved
|
from change #1222: |
bcrochet
force-pushed
the
fixes/1185
branch
2 times, most recently
from
36ccf2f to
8de58b7
Compare
|
from change #1222: |
|
/test 4.17-e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acornett21, bcrochet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
from change #1222: |
This flag deprecates the certification-project-id flag. The field has been changed on the server side, and in certification documentation. Fixes redhat-openshift-ecosystem#1185 Signed-off-by: Brad P. Crochet <[email protected]>
|
New changes are detected. LGTM label has been removed. |
|
from change #1222: |
|
/test 4.17-e2e |
| _ = 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") |
There was a problem hiding this comment.
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.
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. :)
| // 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 { |
There was a problem hiding this comment.
| // 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 { | |
| // 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. | |
| // | |
| // Deprecated: Use WithCertificationComponent instead | |
| func WithCertificationProject(id, token string) Option { |
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.
cmd/preflight/cmd/check_container.go:294:3 staticcheck SA1019: container.WithCertificationProject is deprecated: Use WithCertificationComponent instead
cmd/preflight/cmd/check_container.go:304:17 staticcheck SA1019: container.WithCertificationProject is deprecated: Use WithCertificationComponent instead
There was a problem hiding this comment.
That's a good suggestion. I'll implement that.
| // 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")) | ||
| } |
There was a problem hiding this comment.
Is this only necessary because certification_component_id is bound explicitly to env, separate from the global variable?
| // 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) | ||
| } |
There was a problem hiding this comment.
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.
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.
This flag deprecates the certification-project-id flag. The field has been changed on the server side, and in certification documentation.
Fixes #1185