MG-66: Remove proxy config from CR#313
MG-66: Remove proxy config from CR#313openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@praveencodes: This pull request references MG-66 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
WalkthroughThis PR removes per-CR proxy configuration from the MustGather API and related artifacts; proxy settings are now read exclusively from environment variables (HTTP_PROXY, HTTPS_PROXY, NO_PROXY). Changes include API/type removals, CRD updates, controller simplification, test pruning, docs/examples removal, and generation flag updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (11)
💤 Files with no reviewable changes (6)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (7)
Comment |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
requested review from
TrilokGeer,
rbhilare,
shivprakashmuley and
swghosh
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #313 +/- ##
==========================================
- Coverage 70.98% 68.49% -2.49%
==========================================
Files 7 7
Lines 479 473 -6
==========================================
- Hits 340 324 -16
- Misses 136 144 +8
- Partials 3 5 +2
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
boilerplate/openshift/golang-osd-operator/standard.mk (1)
216-226: Per-directory OpenAPI generation LGTM; watch for future non-API dirs under./api.The updated
openapi-generatetarget correctly runsopenapi-genper API subdirectory and writeszz_generated.openapi.goin place, which matches the newapi/v1alpha1output. Just be aware this will also pick up any future non-Go/non-API directories under./apiand try to runopenapi-genthere; if that ever becomes an issue, consider narrowing thefindto known API package paths.CLAUDE.md (1)
69-72: Proxy documentation matches the new env-based behavior; consider aligning the service account field name with the actual API.The updated prose correctly describes proxy handling as inheriting
HTTP_PROXY/HTTPS_PROXY/NO_PROXYfrom the environment, which matches the controller and CRD changes. One small follow-up for accuracy: the spec field isserviceAccountNamein both Go types and the CRD, but this doc still calls itserviceAccountRefand mentions “ServiceAccountRef from cluster”. Renaming those references toserviceAccountNamewould keep this guide in sync with the actual API surface.Also applies to: 81-87, 110-118, 128-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (11)
CLAUDE.md(4 hunks)README.md(0 hunks)api/v1alpha1/mustgather_types.go(0 hunks)api/v1alpha1/zz_generated.deepcopy.go(0 hunks)api/v1alpha1/zz_generated.openapi.go(1 hunks)boilerplate/openshift/golang-osd-operator/standard.mk(1 hunks)bundle/manifests/tech-preview/operator.openshift.io_mustgathers.yaml(0 hunks)controllers/mustgather/template.go(1 hunks)controllers/mustgather/template_test.go(0 hunks)deploy/crds/operator.openshift.io_mustgathers.yaml(1 hunks)examples/mustgather_proxy.yaml(0 hunks)
💤 Files with no reviewable changes (6)
- api/v1alpha1/mustgather_types.go
- README.md
- api/v1alpha1/zz_generated.deepcopy.go
- controllers/mustgather/template_test.go
- examples/mustgather_proxy.yaml
- bundle/manifests/tech-preview/operator.openshift.io_mustgathers.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/v1alpha1/zz_generated.openapi.goboilerplate/openshift/golang-osd-operator/standard.mkdeploy/crds/operator.openshift.io_mustgathers.yamlCLAUDE.mdcontrollers/mustgather/template.go
🔇 Additional comments (3)
api/v1alpha1/zz_generated.openapi.go (1)
8-14: Empty OpenAPI definition map is consistent with removing ProxySpec.With Proxy-related types removed from the API, returning an empty map from
GetOpenAPIDefinitionsis expected and keeps the generated file minimal. As long as no external component depends on the old ProxySpec OpenAPI schema (which this PR is explicitly removing), this change is fine.deploy/crds/operator.openshift.io_mustgathers.yaml (1)
5-7: CRD schema update correctly dropsspec.proxyConfig; behavior change is coherent with API/controller updates.The CRD now reflects the removal of
spec.proxyConfigand still enforces spec immutability via the existing validation rule, which is consistent with MustGather’s “create-and-let-run” model. This does mean any workloads that previously relied on per-CR proxy settings must now use cluster/operator-level proxy configuration only, but that matches the stated PR goal.Also applies to: 40-174
controllers/mustgather/template.go (1)
54-107: Proxy configuration now sourced solely from operator env; implementation looks consistent with the new API surface.
getJobTemplatenow always deriveshttpProxy,httpsProxy, andnoProxyfrom the operator’s own environment variables and threads those intogetUploadContainer, which only injects them into the upload container when non-empty. This cleanly removes per-CR proxy behavior in favor of a single operator-wide proxy configuration, matching the CRD/API removals and keeping the upload path behavior straightforward.If you haven’t already, it’s worth re-running the existing
templatetests (or adding a focused case) to assert that when the operator pod hasHTTP_PROXY/HTTPS_PROXY/NO_PROXYset, the generated upload container gets the lower-casehttp_proxy/https_proxy/no_proxyenv vars as expected, and that CR-provided values are no longer honored.Also applies to: 205-287
| --output-file zz_generated.openapi.go \ | ||
| --go-header-file /dev/null \ | ||
| --report-filename - \ | ||
| % |
There was a problem hiding this comment.
If we run a boilerplate-update, does this get reverted? I k it's needed, so alternatively we can add another make target in Makefile.
openshift-ci
bot
added
lgtm
|
/override ci/prow/e2e-gcp-operator |
|
@swghosh: Overrode contexts on behalf of swghosh: ci/prow/e2e-gcp-operator 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. |
|
@praveencodes please resole the conflicts. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
@praveencodes: The following test 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. |
|
We'll try to merge this once openshift/enhancements#1903 is approved. |
|
/lgtm /label no-qe |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: praveencodes, swghosh 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 |
|
@praveencodes: This pull request references MG-66 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@praveencodes: This pull request references MG-66 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
/verified bypass |
openshift-ci-robot
added
the
verified
|
@swghosh: The 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-bot
bot
merged commit aedf695
into
openshift:master
6 participants
Removes
spec.proxyConfigas a result of improvements discussed in openshift/enhancements#1903.The operator will observe egress proxy behaviour inherited from OLM env vars directly as a result; operator users (who install through OLM) requiring customization may choose to override these env vars through
Subscription.spec.config.envalternatively.