updating ospid logic to exit if legacy project id is provided #773

acornett21 · 2022-08-22T22:28:32Z

Fixes: certification-project-id with multiple dashes causes 404 status when trying to submit #758
Added a PreRunE to validate the project id, and throw an error if it's in an improper format is used.

Output from test

Examples:
  preflight check container quay.io/repo-name/container-name:version

Flags:
      --certification-project-id string   Certification Project ID from connect.redhat.com/projects/{certification-project-id}/overview
                                          URL paramater. This value may differ from the PID on the overview page. (env: PFLT_CERTIFICATION_PROJECT_ID)
  -h, --help                              help for container
      --pyxis-api-token string            API token for Pyxis authentication (env: PFLT_PYXIS_API_TOKEN)
      --pyxis-env string                  Env to use for Pyxis submissions. (default "prod")
      --pyxis-host string                 Host to use for Pyxis submissions. This will override Pyxis Env. Only set this if you know what you are doing.
                                          If you do set it, it should include just the host, and the URI path. (env: PFLT_PYXIS_HOST)
  -s, --submit                            submit check container results to red hat

Global Flags:
      --artifacts string       Where check-specific artifacts will be written. (env: PFLT_ARTIFACTS)
  -d, --docker-config string   Path to docker config.json file. This value is optional for publicly accessible images.
                               However, it is strongly encouraged for public Docker Hub images,
                               due to the rate limit imposed for unauthenticated requests. (env: PFLT_DOCKERCONFIG)
      --logfile string         Where the execution logfile will be written. (env: PFLT_LOGFILE)
      --loglevel string        The verbosity of the preflight tool itself. Ex. warn, debug, trace, info, error. (env: PFLT_LOGLEVEL)

2022/08/24 16:11:12 certification project id: ospid-62423-f26c346-6cc1dc7fae92 is improperly formatted see help command for instructions on obtaining proper value

Signed-off-by: Adam D. Cornett adc@redhat.com

dcibot · 2022-08-22T22:43:16Z

SUCCESS https://www.distributed-ci.io/jobs/8d564b6c-00ab-4bf5-ada8-23fb5b1d0884/jobStates
from updating ospid logic to exit if legacy project id is provided #773

bcrochet

Couple of suggestions..

bcrochet · 2022-08-23T12:15:57Z

certification/runtime/config.go

+		// counting the `-` values, if the user provides more than one, we know they inputted a legacy project id
+		// which can not be used to query pyxis, exiting early to show the help menu
+		if strings.Count(certificationProjectID, "-") != 1 {
+			return errors.Errorf("certification prodject id: %s is improperly formatted see help for instructions on obtaining proper value", certificationProjectID)


Suggested change

return errors.Errorf("certification prodject id: %s is improperly formatted see help for instructions on obtaining proper value", certificationProjectID)

return errors.Errorf("certification prodject id: %s is improperly formatted, see instructions to obtain the proper value", certificationProjectID)

But also... Where are these instructions? A link might help.

We only have that info in our help output, I believe.

certification/runtime/config.go

komish · 2022-08-23T13:47:05Z

certification/runtime/config.go

 	return &cfg, nil
 }

 // storeContainerPolicyConfiguration reads container-policy-specific config
 // items in viper, normalizes them, and stores them in Config.
-func (c *Config) storeContainerPolicyConfiguration(vcfg viper.Viper) {
+func (c *Config) storeContainerPolicyConfiguration(vcfg viper.Viper) error {
 	c.PyxisAPIToken = vcfg.GetString("pyxis_api_token")
 	c.Submit = vcfg.GetBool("submit")
 	c.PyxisHost = pyxisHostLookup(vcfg.GetString("pyxis_env"), vcfg.GetString("pyxis_host"))

 	// Strip the ospid- prefix from the project ID if provided.
 	certificationProjectID := vcfg.GetString("certification_project_id")
 	if strings.HasPrefix(certificationProjectID, "ospid-") {


Does it make more sense to do these validations before we get here? Like as a PreRunE.

I can see it both ways; it won't take much to sway me one way or the other. Consider this non-blocking. Just thinking out loud.

I don't think that the flags would be loaded into viper if there was a PreRunE, but I might be wrong.

@bcrochet Thoughts on this?

You can add a PreRunE. It should still run the PersistentPreRun. And I agree with @komish. The validation should happen earlier.

For reference: https://github.com/spf13/cobra/blob/main/user_guide.md#prerun-and-postrun-hooks

The flags do get loaded before PreRunE so I have removed the logic from config.go to check_containter.go PTAL.

coveralls · 2022-08-23T14:38:12Z

Coverage increased (+0.03%) to 84.563% when pulling 6627ef0 on acornett21:bugfix_ospid_parsing into 5f4cc98 on redhat-openshift-ecosystem:main.

dcibot · 2022-08-23T16:09:08Z

SUCCESS https://www.distributed-ci.io/jobs/431f1de8-259b-4407-8749-ec0ec72a48c8/jobStates
from updating ospid logic to exit if legacy project id is provided #773

dcibot · 2022-08-23T23:09:20Z

SUCCESS https://www.distributed-ci.io/jobs/3b88e1f9-0d2d-4e09-b57a-1d978f3f7525/jobStates
from updating ospid logic to exit if legacy project id is provided #773

dcibot · 2022-08-24T23:28:11Z

SUCCESS https://www.distributed-ci.io/jobs/64515af8-03fa-46c6-b7db-3cbdc3fa9348/jobStates
from updating ospid logic to exit if legacy project id is provided #773

cmd/check_container.go

bcrochet

Fix the gofumpt error, but otherwise lgtm

acornett21 · 2022-08-25T14:15:12Z

/test 4.8-images

dcibot · 2022-08-25T14:31:33Z

SUCCESS https://www.distributed-ci.io/jobs/77ea640d-825a-4017-84ce-933a508c8a91/jobStates
from updating ospid logic to exit if legacy project id is provided #773

komish

/lgtm

acornett21 · 2022-08-25T20:07:39Z

Putting this on hold, there is another use case that we've never covered. ie a user never gives an ospid but provides a legacy project id of 123-456-789-000. Might as well try to cover that case as well.

…egacy project id Signed-off-by: Adam D. Cornett <adc@redhat.com>

dcibot · 2022-08-25T22:38:02Z

SUCCESS https://www.distributed-ci.io/jobs/fa7924c0-d825-495d-80af-c9814f8570e0/jobStates
from updating ospid logic to exit if legacy project id is provided #773

komish

/lgtm

bcrochet

/lgtm

openshift-ci · 2022-08-30T18:27:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, jomkz, komish

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:

~~OWNERS~~ [acornett21,bcrochet,jomkz,komish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

acornett21 requested review from komish and bcrochet August 22, 2022 22:28

openshift-ci bot requested a review from skattoju August 22, 2022 22:29

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2022

bcrochet reviewed Aug 23, 2022

View reviewed changes

komish reviewed Aug 23, 2022

View reviewed changes

bcrochet reviewed Aug 25, 2022

View reviewed changes

cmd/check_container.go Outdated Show resolved Hide resolved

bcrochet reviewed Aug 25, 2022

View reviewed changes

komish approved these changes Aug 25, 2022

View reviewed changes

openshift-ci bot assigned komish Aug 25, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2022

acornett21 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2022

adding logic to exit early and display the help if the user gives a l…

6627ef0

…egacy project id Signed-off-by: Adam D. Cornett <adc@redhat.com>

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2022

acornett21 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2022

komish approved these changes Aug 26, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2022

bcrochet approved these changes Aug 30, 2022

View reviewed changes

openshift-ci bot assigned bcrochet Aug 30, 2022

jomkz approved these changes Aug 30, 2022

View reviewed changes

openshift-ci bot assigned jomkz Aug 30, 2022

jomkz merged commit 7555dc9 into redhat-openshift-ecosystem:main Aug 30, 2022

acornett21 deleted the bugfix_ospid_parsing branch September 6, 2022 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating ospid logic to exit if legacy project id is provided #773

updating ospid logic to exit if legacy project id is provided #773

acornett21 commented Aug 22, 2022 •

edited

Loading

dcibot commented Aug 22, 2022

bcrochet left a comment

bcrochet Aug 23, 2022

bcrochet Aug 23, 2022

acornett21 Aug 23, 2022

komish Aug 23, 2022

acornett21 Aug 23, 2022

bcrochet Aug 24, 2022 •

edited

Loading

bcrochet Aug 24, 2022

acornett21 Aug 24, 2022

coveralls commented Aug 23, 2022 •

edited

Loading

dcibot commented Aug 23, 2022

dcibot commented Aug 23, 2022

dcibot commented Aug 24, 2022

bcrochet left a comment

acornett21 commented Aug 25, 2022

dcibot commented Aug 25, 2022

komish left a comment

acornett21 commented Aug 25, 2022

dcibot commented Aug 25, 2022

komish left a comment

bcrochet left a comment

openshift-ci bot commented Aug 30, 2022

	return errors.Errorf("certification prodject id: %s is improperly formatted see help for instructions on obtaining proper value", certificationProjectID)
	return errors.Errorf("certification prodject id: %s is improperly formatted, see instructions to obtain the proper value", certificationProjectID)

updating ospid logic to exit if legacy project id is provided #773

updating ospid logic to exit if legacy project id is provided #773

Conversation

acornett21 commented Aug 22, 2022 • edited Loading

dcibot commented Aug 22, 2022

bcrochet left a comment

Choose a reason for hiding this comment

bcrochet Aug 23, 2022

Choose a reason for hiding this comment

bcrochet Aug 23, 2022

Choose a reason for hiding this comment

acornett21 Aug 23, 2022

Choose a reason for hiding this comment

komish Aug 23, 2022

Choose a reason for hiding this comment

acornett21 Aug 23, 2022

Choose a reason for hiding this comment

bcrochet Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

bcrochet Aug 24, 2022

Choose a reason for hiding this comment

acornett21 Aug 24, 2022

Choose a reason for hiding this comment

coveralls commented Aug 23, 2022 • edited Loading

dcibot commented Aug 23, 2022

dcibot commented Aug 23, 2022

dcibot commented Aug 24, 2022

bcrochet left a comment

Choose a reason for hiding this comment

acornett21 commented Aug 25, 2022

dcibot commented Aug 25, 2022

komish left a comment

Choose a reason for hiding this comment

acornett21 commented Aug 25, 2022

dcibot commented Aug 25, 2022

komish left a comment

Choose a reason for hiding this comment

bcrochet left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 30, 2022

acornett21 commented Aug 22, 2022 •

edited

Loading

bcrochet Aug 24, 2022 •

edited

Loading

coveralls commented Aug 23, 2022 •

edited

Loading