removing the deprecated certification-project-id flag

[acornett21]

Motivation

The flag has been deprecated from sometime now, and it was found out that we were overriding precedences of flags, env, and configs with taking envs over flags. Since there is a bug in the custom logic, that goes against cobra's order, it makes sense to just remove this flag.

Changes

• Removes the certification-project-id flag
• Update supporting vars to reference component flag

Build-depends: redhatci/ansible-collection-redhatci-ocp#810

[coveralls]

coverage: 83.755% (-0.06%) from 83.812%
when pulling 1659820 on acornett21:remove_depricated_flag
into 1a441f2 on redhat-openshift-ecosystem:main.

[github-actions]

Code Review by Gemini

Overall, this pull request successfully addresses the deprecation of the certification-project-id flag by removing it and transitioning to certification-component-id. The changes simplify the flag handling logic and update relevant code paths, documentation, and tests.

However, there are a couple of minor bugs and some unrelated changes that should be addressed for clarity and correctness.

Here's a detailed review:

---

General Feedback

• Motivation and Changes: The commit message clearly explains the motivation (deprecated flag, bug in custom precedence logic) and the changes made. This is excellent.
• Code Simplification: Removing the deprecated flag and the associated custom logic for precedence (e.g., StringVar, manual BindEnv, and the conditional viper.Set calls) significantly simplifies the check_container.go command setup, which is a great improvement.

---

Specific Feedback and Suggestions

1. internal/lib/types.go - NewPyxisClient (Bug)

--- a/internal/lib/types.go
+++ b/internal/lib/types.go
@@ -33,21 +33,21 @@ type ResultSubmitter interface {
 }
 
 // PyxisClient defines pyxis API interactions that are relevant to check executions in cmd.
 type PyxisClient interface {
 	FindImagesByDigest(ctx context.Context, digests []string) ([]pyxis.CertImage, error)
 	GetProject(context.Context) (*pyxis.CertProject, error)
 	SubmitResults(context.Context, *pyxis.CertificationInput) (*pyxis.CertificationResults, error)
 }
 
 // NewPyxisClient initializes a pyxisClient with relevant information from cfg.
-// If the the CertificationProjectID, PyxisAPIToken, or PyxisHost are empty, then nil is returned.
+// If the the CertificationComponentID, PyxisAPIToken, or PyxisHost are empty, then nil is returned.
 // Callers should treat a nil pyxis client as an indicator that pyxis calls should not be made.
-func NewPyxisClient(ctx context.Context, projectID, token, host string) PyxisClient {
-	if projectID == "" || token == "" || host == "" {
+func NewPyxisClient(ctx context.Context, componentID, token, host string) PyxisClient { // Renamed parameter to componentID
+	if componentID == "" || token == "" || host == "" { // Condition uses componentID
 		return nil
 	}
 
 	return pyxis.NewPyxisClient(
 		host,
 		token,
-		projectID, // Still passing projectID here
+		componentID, // Should be componentID

• Feedback: The parameter name in NewPyxisClient's signature was correctly changed from projectID to componentID, and the if condition also uses componentID. However, the actual argument passed to pyxis.NewPyxisClient is still projectID. This is a bug.
• Suggestion: Change projectID to componentID in the pyxis.NewPyxisClient call:

  return pyxis.NewPyxisClient(
      host,
      token,
      componentID, // <-- Change this
  )

2. internal/runtime/config_read.go - CertificationProjectID() method (Inconsistency)

--- a/internal/runtime/config_read.go
+++ b/internal/runtime/config_read.go
@@ -34,21 +34,21 @@ func (ro *ReadOnlyConfig) IsBundle() bool {
 
 func (ro *ReadOnlyConfig) IsScratch() bool {
 	return ro.cfg.Scratch
 }
 
 func (ro *ReadOnlyConfig) LogFile() string {
 	return ro.cfg.LogFile
 }
 
 func (ro *ReadOnlyConfig) CertificationProjectID() string {
-	return ro.cfg.CertificationProjectID
+	return ro.cfg.CertificationComponentID
 }
 
 func (ro *ReadOnlyConfig) PyxisHost() string {
 	return ro.cfg.PyxisHost
 }

• Feedback: The ReadOnlyConfig struct's method is still named CertificationProjectID(), but it now returns the value from ro.cfg.CertificationComponentID. This is inconsistent and can be confusing.
• Suggestion: Rename the method to CertificationComponentID() to match the underlying field and the new flag/env var name.

3. .github/workflows/build-multiarch.yml - cosign-installer Downgrade (Unrelated Change)

--- a/.github/workflows/build-multiarch.yml
+++ b/.github/workflows/build-multiarch.yml
@@ -42,21 +42,21 @@ jobs:
       contents: read
       packages: write
       # This is used to complete the identity challenge
       # with sigstore/fulcio when running outside of PRs.
       id-token: write
     name: create and push a multiarch manifest to the repo
     runs-on: ubuntu-latest
     steps:
       - name: Install cosign
         if: ${{ inputs.sign == true && github.event.release && github.event.action == 'published' }}
-        uses: sigstore/cosign-installer@faadad0cce49287aee09b3a48701e75088a2c6ad #v4.0.0
+        uses: sigstore/cosign-installer@d7543c93d881b35a8faa02e8e3605f69b7a1ce62 #v3.10.0
         with:
           cosign-release: 'v2.2.4'

• Feedback: This change downgrades the sigstore/cosign-installer action from v4.0.0 to v3.10.0. The commit message does not mention this change, and it appears unrelated to the flag removal. Downgrading an action might have unintended consequences or indicate a compatibility issue.
• Suggestion:
  • If this downgrade is intentional and necessary (e.g., due to a bug in v4.0.0 or compatibility with other tools), please add a note to the commit message explaining why.
  • If it's unrelated, it should be reverted or moved to a separate commit to keep the changes focused.

4. internal/policy/operator/deployable_by_olm_test.go - Test Client Setup (Unrelated Change)

--- a/internal/policy/operator/deployable_by_olm_test.go
+++ b/internal/policy/operator/deployable_by_olm_test.go
@@ -63,26 +63,22 @@ var _ = Describe("DeployableByOLMCheck", func() {
 			})
 		})
 		Context("When the supported install modes are OwnNamespace and SingleNamespace", func() {
 			BeforeEach(func() {
 				imageRef.ImageFSPath = "./testdata/own_namespace"
 
 				// changing the namespace since OwnNamespace operators CSV get applied to `InstallNamespace`
 				ownCSV := csv.DeepCopy()
 				ownCSV.Namespace = "p-testPackage"
 
-				scheme := apiruntime.NewScheme()
-				Expect(openshift.AddSchemes(scheme)).To(Succeed())
-				deployableByOLMCheck.client = fake.NewClientBuilder().
-					WithScheme(scheme).
-					WithObjects(&csvDefault, &csvMarketplace, &ns, &secret, &sub, &og, ownCSV).
-					WithLists(&pods, &isList).
+				deployableByOLMCheck.client = clientBuilder.
+					WithObjects(ownCSV).
 					Build()
 			})
 			It("OperatorGroup should use InstallNamespace and Should pass Validate", func() {
 				ok, err := deployableByOLMCheck.Validate(testcontext, imageRef)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(ok).To(BeTrue())
 			})
 		})

• Feedback: This hunk significantly changes how the fake client is built for the DeployableByOLMCheck test. It removes the explicit scheme setup and object list additions, simplifying it to use clientBuilder.WithObjects(ownCSV).Build(). While this might be a valid refactoring or optimization for the test, it's not directly related to the certification-project-id flag removal.
• Suggestion:
  • If this is a general test refactoring, it would be cleaner in a separate commit.
  • If it's necessary due to updated dependencies (e.g., k8s.io/client-go or sigs.k8s.io/controller-runtime), a comment explaining the change would be beneficial.

5. go.mod and go.sum (Dependency Updates)

• Feedback: There are several dependency updates across various modules (e.g., docker/cli, ginkgo/v2, gomega, openshift/api, client-go, operator-framework/api, spf13/viper, k8s.io/api, apimachinery, client-go, controller-runtime).
• Suggestion: While keeping dependencies updated is good, large sets of dependency updates are often best handled in separate commits from feature/bug fixes to isolate potential issues and make reviews easier. For this PR, it's acceptable if these are routine updates and don't introduce breaking changes relevant to the core logic.

---

Conclusion

The core changes to remove the deprecated flag are well-executed and improve the codebase by simplifying flag handling and adhering to Viper's precedence rules.

Please address the two identified bugs/inconsistencies in internal/lib/types.go and internal/runtime/config_read.go. Also, consider clarifying or separating the unrelated changes in the GitHub workflow and the operator test file.

[dcibot]

from change #1318:

• FAILURE https://www.distributed-ci.io/jobs/1ce31671-7b9c-44d2-830c-39da16b81596/jobStates

[dcibot]

from change #1318:

• FAILURE https://www.distributed-ci.io/jobs/85284871-bc29-42d2-9a6a-956ec0bb9441/jobStates

[dcibot]

from change #1318:

• FAILURE https://www.distributed-ci.io/jobs/46671db7-2a70-415c-9126-39d136c51b65/jobStates

[github-actions]

Code Review by Gemini

The changes in this pull request focus on removing the deprecated certification-project-id flag and its associated logic, replacing it entirely with certification-component-id. This is a positive change that simplifies the codebase and addresses a known bug related to flag/environment variable precedence.

However, there are a few significant concerns that need to be addressed before this PR can be merged.

---

General Feedback

Positive Aspects:

1. Clear Motivation: The commit message clearly explains the motivation for removing the deprecated flag and the associated bug in precedence logic.
2. Code Simplification: The removal of the certification-project-id flag, its deprecation notice, the MarkFlagsMutuallyExclusive call, and the custom precedence logic in checkContainerPositionalArgs significantly simplifies the command-line argument parsing. This is a great improvement for maintainability and correctness.
3. Consistent Renaming: The refactoring to consistently use "component ID" instead of "project ID" across variable names, function parameters, and configuration fields (Config struct, NewPyxisClient, generateContainerCheckOptions, validateCertificationComponentID) is well-executed.
4. Documentation Update: The docs/CONFIG.md file is correctly updated to reflect the new environment variable name.
5. Test Updates: The test cases in check_container_test.go are appropriately updated to remove references to the old flag and validate the new behavior.

---

Specific Feedback and Suggestions for Improvement

1. Critical: Dependency Downgrades in go.mod and go.sum

• Observation: A large number of dependencies, including core Kubernetes (k8s.io/*), OpenShift (openshift/*), and controller-runtime (sigs.k8s.io/controller-runtime) libraries, as well as github.com/docker/cli, spf13/* packages, and testing frameworks (ginkgo, gomega), have been downgraded. Additionally, the sigstore/cosign-installer GitHub Action in build-multiarch.yml was downgraded from v4.0.0 to v3.10.0.
• Concern: Downgrading dependencies can reintroduce bugs, security vulnerabilities, or compatibility issues that were fixed in newer versions. This is a significant risk and is not mentioned in the commit message. It's crucial to understand why these downgrades occurred.
• Recommendation:
  • Investigate: Determine if these downgrades were intentional. If so, provide a clear justification (e.g., compatibility with a specific Go version, a breaking change in a newer dependency that needs more work).
  • Revert/Update: If unintentional (e.g., due to rebasing onto an older branch, or an issue with go mod tidy), revert go.mod and go.sum to their state before these changes, or update them to the latest compatible versions using go get -u ./... and go mod tidy.
  • Transparency: Any dependency changes, especially downgrades, should be explicitly mentioned and justified in the pull request description.

2. Minor: Misleading Method Name in internal/runtime/config_read.go

• Observation: The method ReadOnlyConfig.CertificationProjectID() now returns the value of ro.cfg.CertificationComponentID.

• Concern: The method name CertificationProjectID() is now misleading, as it returns a component ID, not a project ID. This can lead to confusion for callers.

• Recommendation: Rename the method CertificationProjectID() to CertificationComponentID() in internal/runtime/config_read.go. This will require updating any internal callers of this method and the corresponding test case in internal/runtime/config_read_test.go.

  --- a/internal/runtime/config_read.go
  +++ b/internal/runtime/config_read.go
  @@ -34,7 +34,7 @@ func (ro *ReadOnlyConfig) IsBundle() bool {
   	return ro.cfg.Scratch
   }
   
  -func (ro *ReadOnlyConfig) CertificationProjectID() string {
  -	return ro.cfg.CertificationProjectID
  +func (ro *ReadOnlyConfig) CertificationComponentID() string {
  +	return ro.cfg.CertificationComponentID
   }
   
   func (ro *ReadOnlyConfig) PyxisHost() string {

3. Minor: Test Client Setup in internal/policy/operator/deployable_by_olm_test.go

• Observation: The test setup for deployableByOLMCheck.client was simplified by removing explicit WithScheme and WithLists calls, relying on clientBuilder.
• Recommendation: Ensure that clientBuilder is consistently and correctly initialized elsewhere (e.g., in a BeforeSuite or BeforeEach block for the test suite) to include all necessary schemes and objects for the tests to remain robust. If clientBuilder is a global or package-level variable, its setup should be clear. This is likely fine, but worth a quick check.

---

Conclusion

The core logic changes to remove the deprecated flag are well-implemented and improve the codebase. However, the widespread dependency downgrades are a significant concern that must be addressed. Once the dependency issues are resolved and the minor naming inconsistency is fixed, this PR will be in good shape.

[github-actions]

Code Review by Gemini

The changes effectively remove the deprecated certification-project-id flag and fully transition to certification-component-id. This simplifies the codebase, removes custom logic that was overriding Cobra's precedence rules, and improves clarity.

Here's a detailed review:

General Observations

• Motivation: The motivation for these changes is clear and well-articulated in the commit message. Removing deprecated code and fixing precedence issues are excellent reasons for this refactoring.
• Completeness: The changes appear comprehensive, touching on flag definitions, Viper bindings, configuration structs, validation logic, documentation, and tests.

Specific Feedback and Suggestions

1. internal/runtime/config_read.go - Method Name Inconsistency

• Issue: The ReadOnlyConfig struct still has a method named CertificationProjectID(), but it now returns the value of ro.cfg.CertificationComponentID. This creates a confusing discrepancy between the method name and the data it returns.

• Suggestion: Rename the method CertificationProjectID() to CertificationComponentID() in internal/runtime/config_read.go to align with the new field name and terminology. This will improve code clarity and maintainability.

  --- a/internal/runtime/config_read.go
  +++ b/internal/runtime/config_read.go
  @@ -34,7 +34,7 @@ func (ro *ReadOnlyConfig) IsBundle() bool {
   	return ro.cfg.Scratch
   }
   
   func (ro *ReadOnlyConfig) LogFile() string {
   	return ro.cfg.LogFile
   }
   
  -func (ro *ReadOnlyConfig) CertificationProjectID() string {
  -	return ro.cfg.CertificationProjectID
  +func (ro *ReadOnlyConfig) CertificationComponentID() string {
  +	return ro.cfg.CertificationComponentID
   }
   
   func (ro *ReadOnlyConfig) PyxisHost() string {

  • Impact: This would require updating any calls to ReadOnlyConfig.CertificationProjectID() throughout the codebase to ReadOnlyConfig.CertificationComponentID(). Based on the diff, it seems cfg.CertificationProjectID was directly accessed in check_container.go and lib/types.go, so the impact might be minimal, but it's worth a global search.

2. Adherence to Best Practices

• Removal of Deprecated Code: Excellent. This reduces technical debt and simplifies the codebase.
• Fixing Precedence Logic: The removal of the custom logic in checkContainerPositionalArgs that manually handled certification-component-id overriding certification-project-id and environment variables is a significant improvement. It aligns with Cobra/Viper's standard precedence rules (flags > env > config), which is a best practice and makes the configuration more predictable.
• Renaming: Renaming variables, functions, and struct fields (CertificationProjectID to CertificationComponentID) consistently across the codebase is very good for clarity.
• Documentation: Updating docs/CONFIG.md is crucial and well-executed.
• Testing: The test updates in check_container_test.go correctly reflect the removal of the old flag and its associated logic, ensuring that the new behavior is properly validated.

3. Security Vulnerabilities

• No new security vulnerabilities are introduced. The changes simplify configuration handling, which generally reduces the risk of misconfiguration.

4. Performance Optimizations

• No direct performance optimizations are made, nor are any regressions introduced. The changes are primarily about code clarity and maintainability.

5. Idiomatic Go

• The code remains idiomatic Go. The use of Cobra for command-line parsing and Viper for configuration management is standard for this type of application. The refactoring makes the configuration handling more straightforward and less "hacky" by removing the custom precedence logic.

Conclusion

The changes are well-executed and address the stated motivation effectively. The removal of the deprecated flag and the simplification of configuration precedence are significant improvements. The only notable suggestion is to rename the CertificationProjectID() method in internal/runtime/config_read.go for consistency.

Overall Approval: Looks good, with a minor suggestion for a method rename.

[github-actions]

Code Review by Gemini

The changes effectively remove the deprecated certification-project-id flag and fully transition to certification-component-id. This is a well-executed cleanup that also addresses a reported bug regarding flag/env/config precedence.

Here's a detailed review:

General Feedback

• Motivation: The commit message clearly explains the motivation, which is excellent. Removing deprecated code and fixing precedence issues are both strong reasons for this change.
• Completeness: The changes are comprehensive, touching all relevant files: command definitions, tests, documentation, interfaces, and runtime configuration.
• Clarity: The code is now clearer by removing the transitional logic that handled both flags and their precedence.

Specific Feedback and Suggestions

cmd/preflight/cmd/check_container.go

• Flag Removal: The removal of the certification-project-id flag, its MarkDeprecated call, and the MarkFlagsMutuallyExclusive call is correct and cleans up the command definition.
• Precedence Logic Removal: The removal of the custom logic in checkContainerPositionalArgs that manually handled certification-component-id overriding certification-project-id is a significant improvement. By directly binding certification-component-id to Viper using _ = viper.BindPFlag("certification_component_id", flags.Lookup("certification-component-id")), you are now correctly leveraging Viper's built-in precedence rules (flags > env vars > config files), which directly addresses the "bug in the custom logic, that goes against cobra's order" mentioned in the commit body. This is a great win for maintainability and correctness.
• Renaming: All instances of validateCertificationProjectID have been correctly renamed to validateCertificationComponentID, and internal variable names and Viper keys have been updated accordingly.
• Error Messages: Error messages have been updated to refer to "certification component ID," which is good for user clarity.

cmd/preflight/cmd/check_container_test.go

• Test Updates: The test file has been thoroughly updated to remove tests related to the deprecated flag and its precedence logic. Existing tests have been adjusted to use certification-component-id and reflect the new expected behavior and error messages. This ensures the new logic is correctly validated.
• Test Data Removal: The testdata/project/config.yaml file, which contained the deprecated certification_project_id, has been correctly removed.

docs/CONFIG.md

• Documentation Update: The documentation has been updated to reflect the removal of PFLT_CERTIFICATION_PROJECT_ID and the sole reliance on PFLT_CERTIFICATION_COMPONENT_ID. This is crucial for users.

internal/config/config.go

• Interface Update: The containerConfig interface has been correctly updated to CertificationComponentID() string.

internal/lib/types.go

• Minor Suggestion (Idiomatic Go): In the NewPyxisClient function, the parameter projectID is now receiving the CertificationComponentID. While functionally correct, for better clarity and idiomatic Go, it would be slightly better to rename the parameter from projectID to componentID to match its actual content.

  --- a/internal/lib/types.go
  +++ b/internal/lib/types.go
  @@ -41,10 +41,10 @@ type PyxisClient interface {
   
   // NewPyxisClient initializes a pyxisClient with relevant information from cfg.
   // If the the CertificationComponentID, PyxisAPIToken, or PyxisHost are empty, then nil is returned.
   // Callers should treat a nil pyxis client as an indicator that pyxis calls should not be made.
  -func NewPyxisClient(ctx context.Context, projectID, token, host string) PyxisClient {
  -	if projectID == "" || token == "" || host == "" {
  +func NewPyxisClient(ctx context.Context, componentID, token, host string) PyxisClient {
  +	if componentID == "" || token == "" || host == "" {
   		return nil
   	}
   
   	return pyxis.NewPyxisClient(
  -		host,
  -		token,
  -		projectID,
  +		host,
  +		token,
  +		componentID,
   		logr.FromContextOrDiscard(ctx),
   	)
   }

  This is a minor nit and doesn't affect correctness, but it improves readability.

internal/runtime/config.go, internal/runtime/config_read.go, internal/runtime/config_read_test.go, internal/runtime/config_test.go

• Struct and Method Renaming: The Config struct field, its getter method in ReadOnlyConfig, and all related tests have been correctly updated from CertificationProjectID to CertificationComponentID. This ensures consistency throughout the application's internal representation of the configuration.

Security Vulnerabilities

• No new vulnerabilities introduced: The changes primarily involve removing deprecated code and simplifying configuration parsing. By aligning with Cobra/Viper's standard precedence, the risk of custom logic bugs (which could potentially be exploited) is reduced. This is a positive security outcome.

Adherence to Best Practices

• Removing Dead Code: Excellent adherence to the best practice of removing deprecated and unused code.
• Configuration Management: Moving away from custom precedence logic to Viper's built-in mechanism is a significant improvement in best practices for configuration management in Go applications using Cobra/Viper.
• Documentation: Updating user-facing documentation is a good practice.

Performance Optimizations

• Negligible Impact: The changes are primarily refactoring and cleanup. There's no noticeable performance impact, positive or negative. The removal of some conditional logic might slightly improve execution time, but it would be negligible.

Idiomatic Go

• The code remains idiomatic Go. The use of cobra, viper, and standard library features is consistent with existing patterns in the codebase. The only minor suggestion for idiomatic improvement is the parameter renaming in NewPyxisClient as mentioned above.

Conclusion

This is a well-executed and beneficial change. It cleans up the codebase, removes deprecated functionality, and improves the robustness of configuration parsing by leveraging standard library features.

Recommendation: Approve with the minor suggestion for renaming the projectID parameter in NewPyxisClient for improved clarity, if desired. If not, the current implementation is still functionally correct.

[dcibot]

from change #1318:

• FAILURE https://www.distributed-ci.io/jobs/10d351ee-bf78-4594-9a72-3875bcdf102d/jobStates

[acornett21]

/dci-rerun

[dcibot]

from change #1318:

• SUCCESS https://www.distributed-ci.io/jobs/5b1d4ff6-1805-4ded-86c4-6a0e677d6555/jobStates

[bcrochet]

Comment removal

[dcibot]

from change #1318:

• SUCCESS https://www.distributed-ci.io/jobs/21351828-f8ee-4057-9a01-537b290b2088/jobStates

[bcrochet]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, caxu-rh

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]

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