draft: clean up environment variables#79
draft: clean up environment variables#79killianmuldoon wants to merge 7 commits intorh-ecosystem-edge:mainfrom
Conversation
move the remaining and overlapping defaulted variables to the env.defaults template Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Drop unused variable from the environment and scripts Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: killianmuldoon The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @killianmuldoon. Thanks for your PR. I'm waiting for a rh-ecosystem-edge member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
WalkthroughThe PR centralizes environment variable defaults from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 1-2: The Makefile conditional mistakenly uses filter to decide
when to skip loading .env, causing .env to be skipped whenever any exempt goal
appears; change the conditional to use filter-out so it only skips .env when ALL
goals are exempt (i.e., replace the filter-based test with a filter-out-based
test), updating the existing conditional block that checks "$(filter
generate-env validate-env-files help,$(MAKECMDGOALS))" to use "$(filter-out
generate-env validate-env-files help,$(MAKECMDGOALS))" so mixed-goal invocations
like "make validate-env-files deploy-dpf" will still load the .env for the
non-exempt targets.
In `@scripts/manifests.sh`:
- Line 137: The script currently always appends "custom-catalogsource.yaml" to
excluded_files, causing the custom catalog source to never be prepared even when
CATALOG_SOURCE_IMAGE is set and deploy_core_operator_sources would deploy it;
update the logic in scripts/manifests.sh so that the exclusion is conditional:
only add "custom-catalogsource.yaml" to the excluded_files array when
CATALOG_SOURCE_IMAGE is unset/empty (check the CATALOG_SOURCE_IMAGE env var),
leaving it included when CATALOG_SOURCE_IMAGE is provided; locate the exclusion
site that mutates excluded_files and adjust the conditional there (reference:
excluded_files, "custom-catalogsource.yaml", CATALOG_SOURCE_IMAGE, and
deploy_core_operator_sources/_all) so prepared manifests match deployment
behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Makefileci/env.defaultsci/env.requiredci/env.templatedocs/user-guide/configuration.mddocs/user-guide/deployment-scenarios.mddocs/user-guide/troubleshooting.mdmanifests/cluster-installation/custom-catalogsource.yamlscripts/env.shscripts/manifests.shscripts/traffic-flow-tests.sh
💤 Files with no reviewable changes (3)
- ci/env.required
- docs/user-guide/configuration.md
- docs/user-guide/deployment-scenarios.md
| # Include environment variables (skip for targets that don't need a .env) | ||
| ifeq ($(filter generate-env validate-env-files help,$(MAKECMDGOALS)),) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Makefile" -type f | head -20Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 85
🏁 Script executed:
cat -n Makefile | head -30Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 1494
🏁 Script executed:
# Search for uses of MAKECMDGOALS and .env handling
rg "MAKECMDGOALS|\.env|generate-env|validate-env" -A 2 -B 2Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 21268
🏁 Script executed:
# Check if there are test files or documentation about Makefile usage
find . -type f \( -name "*test*" -o -name "*README*" -o -name "*.md" \) -exec grep -l "make.*validate-env\|make.*generate-env\|make.*deploy" {} \; | head -10Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 329
🏁 Script executed:
# Search for CI/CD configs and test patterns
find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.sh" \) -path "*/ci/*" -o -path "*/.github/*" | head -20Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 57
🏁 Script executed:
# Look for any shell scripts or docs that show example make invocations
rg "make\s+\w+\s+\w+" --type md --type sh | grep -E "make\s+[a-z-]+\s+[a-z-]+" | head -20Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 57
🏁 Script executed:
# Check if there's a .github directory or CI configuration
find . -type d \( -name ".github" -o -name "ci" -o -name ".gitlab-ci" \) 2>/dev/nullRepository: rh-ecosystem-edge/openshift-dpf
Length of output: 79
🏁 Script executed:
# Look at the actual Makefile targets and see which ones are typically run together
rg "^[a-z].*:" Makefile | head -30Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 685
🏁 Script executed:
# Check if there are any scripts that invoke make with multiple targets
rg "make\s+" --type sh --type bash | head -20Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 103
🏁 Script executed:
# Look for make invocations in shell scripts
rg "make\s+" --type sh | head -20Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 339
🏁 Script executed:
# Check the ci directory structure
ls -la ci/Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 446
🏁 Script executed:
# Look at ci scripts and configs
find ci -type f | head -20Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 150
🏁 Script executed:
# Check if generate-env is ever called with other targets
rg "generate-env|validate-env-files" --type md --type sh | grep -v "\.env" | head -15Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 57
🏁 Script executed:
# Verify the exact filter/filter-out logic behavior with a simple test
cat > /tmp/test_make_logic.mk << 'EOF'
EXEMPT := a b c
GOALS1 := a d
GOALS2 := a
GOALS3 := d
GOALS4 :=
test:
`@echo` "Test 1: GOALS='a d' - should include .env"
`@echo` "filter result: $(filter a b c,a d)"
`@echo` "filter-out result: $(filter-out a b c,a d)"
`@echo` ""
`@echo` "Test 2: GOALS='a' - should skip .env"
`@echo` "filter result: $(filter a b c,a)"
`@echo` "filter-out result: $(filter-out a b c,a)"
`@echo` ""
`@echo` "Test 3: GOALS='d' - should include .env"
`@echo` "filter result: $(filter a b c,d)"
`@echo` "filter-out result: $(filter-out a b c,d)"
`@echo` ""
`@echo` "Test 4: GOALS='' - should include .env"
`@echo` "filter result: $(filter a b c,)"
`@echo` "filter-out result: $(filter-out a b c,)"
EOF
make -f /tmp/test_make_logic.mk testRepository: rh-ecosystem-edge/openshift-dpf
Length of output: 120
🏁 Script executed:
# Let me verify the logic conceptually by examining how the filter function works
# Check if there are actual examples of how targets depend on each other
rg "^(generate-env|validate-env-files|deploy-dpf|all):" -A 5 MakefileRepository: rh-ecosystem-edge/openshift-dpf
Length of output: 997
🏁 Script executed:
# Check for any documentation or comments about the include guard intention
rg -B2 -A2 "Include environment variables" MakefileRepository: rh-ecosystem-edge/openshift-dpf
Length of output: 232
🏁 Script executed:
# Verify what variables deploy-dpf and other targets actually need
rg "\$\(.*\)" Makefile | grep -E "CLUSTER|API|BASE|DOMAIN|KUBECONFIG" | head -10Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 57
Fix .env loading for mixed-goal invocations.
Line 2 currently skips .env if any exempt goal is present. Example: make validate-env-files deploy-dpf will skip .env for deploy-dpf, breaking runtime variables that scripts expect.
The filter function returns only matching goals; when non-empty, the condition is false and .env is excluded. The solution uses filter-out to detect non-exempt goals instead—only skip .env if all goals are exempt.
Proposed fix
-ifeq ($(filter generate-env validate-env-files help,$(MAKECMDGOALS)),)
-include .env
-export
-endif
+EXEMPT_ENV_GOALS := generate-env validate-env-files help
+ifeq ($(MAKECMDGOALS),)
+include .env
+export
+else ifneq ($(filter-out $(EXEMPT_ENV_GOALS),$(MAKECMDGOALS)),)
+include .env
+export
+endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 1 - 2, The Makefile conditional mistakenly uses filter
to decide when to skip loading .env, causing .env to be skipped whenever any
exempt goal appears; change the conditional to use filter-out so it only skips
.env when ALL goals are exempt (i.e., replace the filter-based test with a
filter-out-based test), updating the existing conditional block that checks
"$(filter generate-env validate-env-files help,$(MAKECMDGOALS))" to use
"$(filter-out generate-env validate-env-files help,$(MAKECMDGOALS))" so
mixed-goal invocations like "make validate-env-files deploy-dpf" will still load
the .env for the non-exempt targets.
| if [ "${USE_V419_WORKAROUND}" != "true" ]; then | ||
| excluded_files+=("4.19-cataloguesource.yaml") | ||
| fi | ||
| excluded_files+=("custom-catalogsource.yaml") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Make target wiring =="
rg -nP '^_all:|^deploy-core-operator-sources:|^deploy-dpf:' Makefile -C2
echo
echo "== Catalog source deployment call sites =="
rg -nP 'deploy_core_operator_sources|custom-catalogsource\.yaml|CATALOG_SOURCE_IMAGE' scripts/manifests.sh -C3
rg -nP 'deploy-core-operator-sources|manifests\.sh' Makefile scripts -C2Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 4562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for CATALOG_SOURCE_IMAGE usage/documentation =="
rg -i 'CATALOG_SOURCE_IMAGE' . --type md --type txt --type sh -C2
echo
echo "== Check if deploy-core-operator-sources is documented for mandatory invocation =="
rg -i 'deploy-core-operator-sources|deploy_core_operator_sources' . --type md --type txt -C2
echo
echo "== Check initialization/defaults of CATALOG_SOURCE_IMAGE =="
rg -nP 'CATALOG_SOURCE_IMAGE.*=' scripts -C1Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 1062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check how CATALOG_SOURCE_IMAGE is initialized/sourced =="
rg -nP 'CATALOG_SOURCE_IMAGE.*=' scripts
echo
echo "== Check env file or config sourcing =="
rg -nP '(source|\..*env|config)' scripts/manifests.sh | head -20
echo
echo "== Full context of deploy-core-operator-sources function =="
sed -n '204,226p' scripts/manifests.shRepository: rh-ecosystem-edge/openshift-dpf
Length of output: 134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full deploy_core_operator_sources function context =="
sed -n '204,227p' scripts/manifests.sh
echo
echo "== Check if CATALOG_SOURCE_IMAGE is exported or documented elsewhere =="
rg -i 'catalog' . --type md --type sh -l | head -10Repository: rh-ecosystem-edge/openshift-dpf
Length of output: 1311
Unguaranteed deployment of custom catalog source when CATALOG_SOURCE_IMAGE is set.
Line 137 unconditionally excludes custom-catalogsource.yaml from prepared manifests, while the custom catalog source is only deployed in deploy_core_operator_sources (lines 216-224) when CATALOG_SOURCE_IMAGE is set. Since deploy-core-operator-sources is not in the _all Make target chain, if a user sets CATALOG_SOURCE_IMAGE and runs make or make _all, the subscriptions will reference a catalog source that was never created. Either add deploy-core-operator-sources to the _all dependency chain, make the exclusion conditional on CATALOG_SOURCE_IMAGE being unset, or ensure this requirement is clearly documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/manifests.sh` at line 137, The script currently always appends
"custom-catalogsource.yaml" to excluded_files, causing the custom catalog source
to never be prepared even when CATALOG_SOURCE_IMAGE is set and
deploy_core_operator_sources would deploy it; update the logic in
scripts/manifests.sh so that the exclusion is conditional: only add
"custom-catalogsource.yaml" to the excluded_files array when
CATALOG_SOURCE_IMAGE is unset/empty (check the CATALOG_SOURCE_IMAGE env var),
leaving it included when CATALOG_SOURCE_IMAGE is provided; locate the exclusion
site that mutates excluded_files and adjust the conditional there (reference:
excluded_files, "custom-catalogsource.yaml", CATALOG_SOURCE_IMAGE, and
deploy_core_operator_sources/_all) so prepared manifests match deployment
behavior.
|
PR needs rebase. 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. |
1 participant
This PR cleans up some unused variables and improves the handling of HOSTED_CONTROL_PLANE_NAMESPACE and the catalog version workaround.
Built on top of #78 - and needs that one to merge first.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores