-
Notifications
You must be signed in to change notification settings - Fork 34
Remove hardcoded product name #1565
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
base: main
Are you sure you want to change the base?
Remove hardcoded product name #1565
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors how product names are determined for ACS, TrustedArtifactSigner, and Trustification integrations. A new helper function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lingyzhuang The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
c2893e4 to
9d371c8
Compare
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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/subcmd/integration.go (1)
44-69: Resolve product names once and consider handling resolution failures per-integrationThe wiring of
ProductName(string(integrations.X))into the ACS, TrustedArtifactSigner, and Trustification constructors looks consistent. Two small follow‑ups you may want to consider:
- Resolve each product name once into a local variable before constructing the slice, instead of calling
ProductNameinline three times; this makes error handling and logging per integration clearer if you adopt an error‑returningProductName.- If product resolution can fail in some environments (e.g., missing charts), decide whether you’d rather:
- log and omit the affected integration subcommand, or
- keep the subcommand but let
Runfail with a clean error whenEnableDisableProductis called.This doesn’t block the current change but will make the behavior more predictable and testable once
ProductNamestops panicking.pkg/subcmd/integration_trustedartifactsigner.go (1)
22-22:pNamewiring for TrustedArtifactSigner looks correctThe new
pNamefield is correctly:
- accepted in
NewIntegrationTrustedArtifactSigner,- stored on the struct, and
- used in
RunviaEnableDisableProduct(t.pName, false).Assuming callers pass the resolved product name (as done in
integration.go), this cleanly removes the hardcoded product constant without changing behavior.Also applies to: 52-57, 66-71, 82-85
pkg/subcmd/integration_acs.go (1)
22-22: ACS integration now correctly uses injected product name instead of a constantThe ACS subcommand now:
- stores
pNameon the struct,- uses
a.pNamewhen callingEnableDisableProduct, and- receives
pNamevia the constructor.This aligns with the new
ProductNamehelper and removes the hardcoded product string without altering the run‑time flow.Also applies to: 52-59, 68-73, 82-87
pkg/subcmd/integration_trustification.go (1)
22-22: Trustification integration follows the same configurable product-name patternFor Trustification,
pNameis:
- defined on the struct,
- injected via
NewIntegrationTrustification, and- used in
Runin theEnableDisableProductcall.This mirrors the ACS and TrustedArtifactSigner changes and removes the hardcoded product name cleanly.
Also applies to: 52-59, 68-73, 82-87
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/subcmd/integration.go(3 hunks)pkg/subcmd/integration_acs.go(4 hunks)pkg/subcmd/integration_trustedartifactsigner.go(4 hunks)pkg/subcmd/integration_trustification.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/subcmd/integration.go (4)
pkg/chartfs/chartfs.go (1)
NewChartFSForCWD(164-170)pkg/resolver/dependency.go (1)
NewDependency(105-107)pkg/integrations/manager.go (3)
ACS(25-25)TrustedArtifactSigner(34-34)Trustification(35-35)pkg/subcmd/integration_trustification.go (1)
NewIntegrationTrustification(68-90)
🪛 GitHub Check: SonarCloud Code Analysis
pkg/subcmd/integration.go
[failure] 23-23: Return an error instead of using panic for normal error conditions.
[failure] 33-33: Return an error instead of using panic for normal error conditions.
[failure] 19-19: Return an error instead of using panic for normal error conditions.
9d371c8 to
844ec49
Compare
|
|
@lingyzhuang: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.19-w7mlgTest results analysis<not enabled> OCI Artifact Browser URL<not enabled> |



Add function
ProductNameinintegration.go, get product name from metadata instead of hardcoded.Jira: RHTAP-6037
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.