Downstream testing tokenRef default Restrictions for ApplicationSet A…#1160
Conversation
|
Hi @nmirasch. Thanks for your PR. I'm waiting for a redhat-developer member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBump Go toolchain and many module versions; align CI/Docker builder images; simplify ImageUpdater CR creation; centralize ARGOCD_CLUSTER_CONFIG_NAMESPACES and add ApplicationSet tokenRef strict-mode tests; update health import paths and convert several boolean fields to pointer booleans in tests. ChangesDependency and build updates
E2E Test Updates
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
/ok-to-test |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
nmirasch
force-pushed
the
GITOPS-9735_enforce_tokenref_restric
branch
from
c8626e1 to
0654673
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go.mod (2)
231-263:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix Kubernetes dependency skew in go.mod
replaceblock (v0.35.2 required, v0.34.0 replaced).
go.modrequires corek8s.io/*modules atv0.35.2(lines 27-30), but thereplaceblock pins a broad set ofk8s.io/*modules tov0.34.0(lines 231-262). This can introduce API/type skew across the Kubernetes dependency graph and make the build behavior depend onreplaceside effects rather than the intended versions.Align the
replaceblock tov0.35.2(or remove the replaces if not required).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` around lines 231 - 263, The replace block in go.mod pins many k8s.io/* modules to v0.34.0 causing dependency skew with the required v0.35.2; update the replace entries that reference k8s.io/api, k8s.io/apimachinery, k8s.io/client-go, etc. in the replace block to v0.35.2 (or remove the entire k8s.io/* replace block if it’s unnecessary) so all k8s.io/* modules align with the project’s declared v0.35.2 dependencies and prevent API/type mismatches.
223-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove/align the
golang.org/x/toolsreplaceto prevent tooling drift
go.modtargets Go 1.26.0, but it requiresgolang.org/x/tools v0.44.0(line 183) whilereplaceforcesgolang.org/x/tools v0.35.0(line 223). This forced downgrade can cause analyzer/gopls/IDE behavior to diverge from what the selected module graph expects.Unless there’s a proven blocker, remove the
replaceor bump it to match the selectedx/toolsversion (or document why the downgrade is necessary).golang.org/x/tools => golang.org/x/tools v0.35.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` at line 223, The replace directive forcing golang.org/x/tools to v0.35.0 conflicts with the required golang.org/x/tools v0.44.0 for Go 1.26.0; remove the replace entry for golang.org/x/tools or update it to v0.44.0 (or add a brief comment documenting a justified downgrade) so the module graph and tooling (gopls/analyzers) use the same x/tools version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@go.mod`:
- Around line 231-263: The replace block in go.mod pins many k8s.io/* modules to
v0.34.0 causing dependency skew with the required v0.35.2; update the replace
entries that reference k8s.io/api, k8s.io/apimachinery, k8s.io/client-go, etc.
in the replace block to v0.35.2 (or remove the entire k8s.io/* replace block if
it’s unnecessary) so all k8s.io/* modules align with the project’s declared
v0.35.2 dependencies and prevent API/type mismatches.
- Line 223: The replace directive forcing golang.org/x/tools to v0.35.0
conflicts with the required golang.org/x/tools v0.44.0 for Go 1.26.0; remove the
replace entry for golang.org/x/tools or update it to v0.44.0 (or add a brief
comment documenting a justified downgrade) so the module graph and tooling
(gopls/analyzers) use the same x/tools version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9f6e59a3-4856-4085-93f8-133d5379e099
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.ci-operator.yamlDockerfilego.modopenshift-ci/build-root/Dockerfiletest/openshift/e2e/ginkgo/fixture/application/fixture.gotest/openshift/e2e/ginkgo/parallel/1-046_validate_application_tracking_test.gotest/openshift/e2e/ginkgo/sequential/1-053_validate_argocd_agent_principal_connected_test.gotest/openshift/e2e/ginkgo/sequential/1-102_validate_handle_terminating_namespaces_test.gotest/openshift/e2e/ginkgo/sequential/1-108_alternate_cluster_roles_cluster_scoped_instance_test.gotest/openshift/e2e/ginkgo/sequential/1-120_repo_server_system_ca_trust.go
✅ Files skipped from review due to trivial changes (4)
- Dockerfile
- .ci-operator.yaml
- openshift-ci/build-root/Dockerfile
- test/openshift/e2e/ginkgo/parallel/1-046_validate_application_tracking_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Around line 195-197: Update the stale go.mod replace pins that override the
intended Kubernetes bump: locate the replace directives referencing
k8s.io/cli-runtime and k8s.io/component-helpers (and any other k8s.io/* replace
entries) and either update their target versions to v0.35.2 to match the require
lines or remove the unnecessary replace directives so the module graph resolves
to the required v0.35.2 versions consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| k8s.io/cli-runtime v0.35.2 // indirect | ||
| k8s.io/component-base v0.35.2 // indirect | ||
| k8s.io/component-helpers v0.34.0 // indirect | ||
| k8s.io/component-helpers v0.35.2 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate go.mod and print relevant sections with line numbers
ls -la
if [ -f go.mod ]; then
echo "== go.mod =="
nl -ba go.mod | sed -n '160,280p'
else
echo "go.mod not found at repo root"
exit 1
fiRepository: redhat-developer/gitops-operator
Length of output: 2287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod: lines 185-210 =="
awk 'NR>=185 && NR<=210 {printf "%d\t%s\n", NR, $0}' go.mod
echo
echo "== go.mod: lines 220-250 =="
awk 'NR>=220 && NR<=250 {printf "%d\t%s\n", NR, $0}' go.mod
echo
echo "== go.mod: matches for k8s.io/cli-runtime =="
rg -n "k8s\.io/cli-runtime" go.mod || true
echo
echo "== go.mod: matches for k8s.io/component-helpers =="
rg -n "k8s\.io/component-helpers" go.mod || true
echo
echo "== go.mod: replace block for k8s.io/* (if any) =="
rg -n "^replace\s+k8s\.io/" go.mod || trueRepository: redhat-developer/gitops-operator
Length of output: 3532
Kubernetes version bump is neutralized by stale replace pins in go.mod.
In go.mod (requires at lines 195-197), k8s.io/cli-runtime and k8s.io/component-helpers are set to v0.35.2, but the Kubernetes replace block pins them back to v0.34.0 (k8s.io/cli-runtime => ... v0.34.0, k8s.io/component-helpers => ... v0.34.0). Because replace takes precedence over require, the upgrade intent is defeated and version skew risk increases.
Align the k8s.io/* replace entries to v0.35.2 (or remove unnecessary replace directives) so the module graph resolves consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` around lines 195 - 197, Update the stale go.mod replace pins that
override the intended Kubernetes bump: locate the replace directives referencing
k8s.io/cli-runtime and k8s.io/component-helpers (and any other k8s.io/* replace
entries) and either update their target versions to v0.35.2 to match the require
lines or remove the unnecessary replace directives so the module graph resolves
to the required v0.35.2 versions consistently.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go.mod (1)
26-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKubernetes dependency bump is effectively overridden by the
replaceblock.
go.moddeclares severalk8s.io/*modules atv0.35.2(e.g., Line 26–Line 29, Line 194–Line 196), but Line 230–Line 261 force those modules back tov0.34.0viareplace. This creates dependency skew and defeats the intended upgrade in this PR.Suggested minimal fix
- k8s.io/api => k8s.io/api v0.34.0 - k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.34.0 - k8s.io/apimachinery => k8s.io/apimachinery v0.34.0 - k8s.io/apiserver => k8s.io/apiserver v0.34.0 - k8s.io/cli-runtime => k8s.io/cli-runtime v0.34.0 - k8s.io/client-go => k8s.io/client-go v0.34.0 + k8s.io/api => k8s.io/api v0.35.2 + k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.35.2 + k8s.io/apimachinery => k8s.io/apimachinery v0.35.2 + k8s.io/apiserver => k8s.io/apiserver v0.35.2 + k8s.io/cli-runtime => k8s.io/cli-runtime v0.35.2 + k8s.io/client-go => k8s.io/client-go v0.35.2 ... - k8s.io/component-base => k8s.io/component-base v0.34.0 - k8s.io/component-helpers => k8s.io/component-helpers v0.34.0 + k8s.io/component-base => k8s.io/component-base v0.35.2 + k8s.io/component-helpers => k8s.io/component-helpers v0.35.2(Or remove unnecessary
k8s.io/*replaces entirely if no longer needed.)As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 194-196, 230-261
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` around lines 26 - 29, The go.mod declares k8s.io/* modules at v0.35.2 but a replace block later pins those same modules to v0.34.0, which negates the upgrade; remove or update the replace entries that reference the k8s.io modules (the replace entries forcing v0.34.0) so they either point to v0.35.2 or are deleted if no longer needed, ensuring the declared module versions (k8s.io/api, k8s.io/apiextensions-apiserver, k8s.io/apimachinery, k8s.io/client-go) take effect and avoid dependency skew.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@go.mod`:
- Around line 26-29: The go.mod declares k8s.io/* modules at v0.35.2 but a
replace block later pins those same modules to v0.34.0, which negates the
upgrade; remove or update the replace entries that reference the k8s.io modules
(the replace entries forcing v0.34.0) so they either point to v0.35.2 or are
deleted if no longer needed, ensuring the declared module versions (k8s.io/api,
k8s.io/apiextensions-apiserver, k8s.io/apimachinery, k8s.io/client-go) take
effect and avoid dependency skew.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8253bc5d-381a-40a9-a80c-8c5310a44daa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
Dockerfilego.modtest/openshift/e2e/ginkgo/fixture/application/fixture.gotest/openshift/e2e/ginkgo/fixture/argocd/fixture.gotest/openshift/e2e/ginkgo/parallel/1-008_validate-custom-argocd-namespace_test.gotest/openshift/e2e/ginkgo/parallel/1-009_validate-manage-other-namespace_test.gotest/openshift/e2e/ginkgo/parallel/1-012_validate-managed-by-chain_test.gotest/openshift/e2e/ginkgo/parallel/1-023_validate_repo_server_tls_test.gotest/openshift/e2e/ginkgo/parallel/1-025-validate-managed-by-change_test.gotest/openshift/e2e/ginkgo/parallel/1-054_validate_deploymentconfig_test.gotest/openshift/e2e/ginkgo/parallel/1-058_validate_prometheus_rule_test.gotest/openshift/e2e/ginkgo/parallel/1-070_validate_config_management_plugin_test.gotest/openshift/e2e/ginkgo/parallel/1-116_annotation-based-tracking_test.gotest/openshift/e2e/ginkgo/parallel/1-121_validate_image_updater_test.gotest/openshift/e2e/ginkgo/parallel/1-122_validate_user_defined_appproject.gotest/openshift/e2e/ginkgo/sequential/1-006_validate_machine_config_test.gotest/openshift/e2e/ginkgo/sequential/1-008_validate-4.9CI-Failures_test.gotest/openshift/e2e/ginkgo/sequential/1-010_validate-ootb-manage-other-namespace_test.gotest/openshift/e2e/ginkgo/sequential/1-027_validate_operand_from_git_test.gotest/openshift/e2e/ginkgo/sequential/1-037_validate_applicationset_in_any_namespace_test.gotest/openshift/e2e/ginkgo/sequential/1-053_validate_argocd_agent_principal_connected_test.gotest/openshift/e2e/ginkgo/sequential/1-059_validate_argocd_agent_terminal_streaming_test.gotest/openshift/e2e/ginkgo/sequential/1-064_validate_tcp_reset_error_test.gotest/openshift/e2e/ginkgo/sequential/1-108_alternate_cluster_roles_cluster_scoped_instance_test.gotest/openshift/e2e/ginkgo/sequential/1-113_validate_namespacemanagement_test.go
✅ Files skipped from review due to trivial changes (13)
- test/openshift/e2e/ginkgo/parallel/1-058_validate_prometheus_rule_test.go
- test/openshift/e2e/ginkgo/parallel/1-012_validate-managed-by-chain_test.go
- test/openshift/e2e/ginkgo/sequential/1-027_validate_operand_from_git_test.go
- test/openshift/e2e/ginkgo/parallel/1-070_validate_config_management_plugin_test.go
- test/openshift/e2e/ginkgo/parallel/1-116_annotation-based-tracking_test.go
- test/openshift/e2e/ginkgo/sequential/1-008_validate-4.9CI-Failures_test.go
- test/openshift/e2e/ginkgo/sequential/1-064_validate_tcp_reset_error_test.go
- test/openshift/e2e/ginkgo/sequential/1-113_validate_namespacemanagement_test.go
- Dockerfile
- test/openshift/e2e/ginkgo/parallel/1-122_validate_user_defined_appproject.go
- test/openshift/e2e/ginkgo/parallel/1-009_validate-manage-other-namespace_test.go
- test/openshift/e2e/ginkgo/sequential/1-059_validate_argocd_agent_terminal_streaming_test.go
- test/openshift/e2e/ginkgo/fixture/application/fixture.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/openshift/e2e/ginkgo/parallel/1-121_validate_image_updater_test.go
- test/openshift/e2e/ginkgo/sequential/1-108_alternate_cluster_roles_cluster_scoped_instance_test.go
- test/openshift/e2e/ginkgo/sequential/1-053_validate_argocd_agent_principal_connected_test.go
- test/openshift/e2e/ginkgo/sequential/1-037_validate_applicationset_in_any_namespace_test.go
nmirasch
force-pushed
the
GITOPS-9735_enforce_tokenref_restric
branch
from
473d4ce to
9720f7b
Compare
…ny-Namespace Mode Assisted-by: Cursor Signed-off-by: nmirasch <neus.miras@gmail.com>
nmirasch
force-pushed
the
GITOPS-9735_enforce_tokenref_restric
branch
from
9720f7b to
b3da9fd
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: svghadi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick v1.21 |
|
@svghadi: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-bot
Bot
merged commit 62c153e
into
redhat-developer:master
|
@svghadi: new pull request created: #1169 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@nmirasch: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
…ny-Namespace Mode
What type of PR is this?
What does this PR do / why we need it:
Update the argocd-operator dependency to argoproj-labs/argocd-operator@e8e51ab, which enforces
ApplicationSet tokenRef strict mode defaults via argocd-cmd-params-cm when
applicationSet sourceNamespaces are configured.
Add downstream e2e coverage in 1-037 to verify the cmd-params key defaults
to true/false based on sourceNamespaces and can be overridden via
spec.cmdParams. Adjust the ImageUpdater test for argocd-image-updater v1.2.0.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes: https://redhat.atlassian.net/browse/GITOPS-9735
Test acceptance criteria:
How to test changes / Special notes to the reviewer: