NO-JIRA: Add manifests verify target #579
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@RadekManak: This pull request explicitly references no jira issue. In 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. |
WalkthroughAdded Makefile targets Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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
🧹 Nitpick comments (2)
openshift/verify-diff.sh (1)
1-11: Consider adding defensive script options for robustness.Adding
set -eorset -euo pipefailat the top of the script would catch unexpected failures early and prevent silent continuation on errors. This is a best practice for verification scripts.Apply this diff to add defensive scripting:
#!/bin/bash +set -euo pipefail FILE_DIFF=$(git ls-files -o --exclude-standard)openshift/Makefile (1)
22-22: Verify robustness of yq extraction for version label.The yq command at line 22 depends on the exact key path
.metadata.labels."provider.cluster.x-k8s.io/version"and file path. Consider documenting the expected manifest structure or adding error handling (e.g., check if extraction succeeds) to provide clearer failure messages if the manifest format changes.If this warrants defensive coding, consider:
@echo "Extracting provider version from existing manifest..." -$(eval PROVIDER_VERSION := $(shell yq eval '.metadata.labels."provider.cluster.x-k8s.io/version"' manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml)) +$(eval PROVIDER_VERSION := $(shell yq eval '.metadata.labels."provider.cluster.x-k8s.io/version"' manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml || echo "")) +ifndef PROVIDER_VERSION + $(error Failed to extract PROVIDER_VERSION from manifest) +endifHowever, this may be overkill if manifest stability is guaranteed.
📜 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 (2)
openshift/Makefile(1 hunks)openshift/verify-diff.sh(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
openshift/Makefile
[warning] 15-15: Target "verify" should be declared PHONY.
(phonydeclared)
RadekManak
force-pushed
the
verify-ocp-manifests
branch
from
686cbdd to
5ccde04
Compare
openshift/Makefile
Outdated
There was a problem hiding this comment.
Isn't this a cyclic dependency?
Like if we did not run make manfiest-gen we might use the old version here?
There was a problem hiding this comment.
Yes. If someone runs this instead of ocp-manfiests it will update the manifests, keep the old version number, print diff, error. Someone could theoretically commit that, and then it would pass in CI.
What about restricting this target to CI using environment variable?
There was a problem hiding this comment.
@chrischdi I updated this to restore the manifest file if there is a diff. That way, it cannot be committed by accident.
RadekManak
force-pushed
the
verify-ocp-manifests
branch
from
5ccde04 to
96a0553
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openshift/Makefile (1)
27-27: Line 27 is overly complex; refactor for readability.The single-line conditional with nested parentheses, repeated
git restorecalls, and chained echo messages is difficult to parse and maintain. Consider splitting into a helper script or multi-line conditional for clarity.Example structure:
verify-ocp-manifests: @echo "Extracting provider version from existing manifest..." $(eval PROVIDER_VERSION := $(shell yq eval '.metadata.labels."provider.cluster.x-k8s.io/version"' $(MANIFEST_FILE))) @echo "Using PROVIDER_VERSION=$(PROVIDER_VERSION) for verification" @$(MAKE) ocp-manifests PROVIDER_VERSION=$(PROVIDER_VERSION) @if ./verify-diff.sh; then \ git restore $(MANIFEST_FILE); \ echo "Verification successful! Manifests are in sync."; \ else \ git restore $(MANIFEST_FILE); \ echo "Verification failed! Manifests are out of sync."; \ exit 1; \ fiAdditionally, extract the hardcoded manifest path to a variable (e.g.,
MANIFEST_FILE) to reduce duplication.
📜 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 (2)
openshift/Makefile(1 hunks)openshift/verify-diff.sh(1 hunks)
🧰 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:
openshift/verify-diff.shopenshift/Makefile
🔇 Additional comments (1)
openshift/verify-diff.sh (1)
1-11: LGTM—variable quoting issue resolved.The previous review's concern about quoting
$FILE_DIFFhas been addressed in line 7. The script correctly checks for untracked files and unstaged changes with proper shell quoting.
openshift/Makefile
Outdated
| .PHONY: verify | ||
| verify: verify-ocp-manifests | ||
|
|
||
| # NOTE: This target extracts the provider version from the existing generated manifest | ||
| # and uses it for regeneration. This means the version label itself is NOT verified, | ||
| # only that the rest of the manifest structure is correct. | ||
| .PHONY: verify-ocp-manifests | ||
| verify-ocp-manifests: | ||
| @echo "Extracting provider version from existing manifest..." | ||
| $(eval PROVIDER_VERSION := $(shell yq eval '.metadata.labels."provider.cluster.x-k8s.io/version"' manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml)) | ||
| @echo "Using PROVIDER_VERSION=$(PROVIDER_VERSION) for verification" | ||
| @$(MAKE) ocp-manifests PROVIDER_VERSION=$(PROVIDER_VERSION) | ||
| @./verify-diff.sh && (git restore manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml; echo -e "\nVerification successful! Manifests are in sync.") || (git restore manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml; echo -e "\nVerification failed! Manifests are out of sync.\nPlease run: PROVIDER_VERSION=UPSTREAM_RELEASE_VERSION make ocp-manifests\n"; exit 1) |
There was a problem hiding this comment.
Design flaw: verify-ocp-manifests can run in isolation, bypassing intent.
As raised in the past review, verify-ocp-manifests can be invoked directly without first running ocp-manifests with a fresh PROVIDER_VERSION. This allows regenerating manifests with a stale version extracted from the existing manifest, making the diff check meaningless—it will pass even if the provider version is outdated. The workflow assumes proper ordering but doesn't enforce it.
Consider one of these mitigations:
- Require CI-only execution: Add a guard checking an environment variable (e.g.,
CI=true) and fail if not set. - Explicit prerequisite: Make
verify-ocp-manifestsdepend on a target that ensures PROVIDER_VERSION is freshly provided (not extracted).
🤖 Prompt for AI Agents
In openshift/Makefile around lines 15 to 27, the verify-ocp-manifests target
extracts PROVIDER_VERSION from the existing manifest which allows running
verification with a stale version; to fix this, add a guard so the target fails
unless executed in CI (e.g., require CI=true) AND/OR require an explicit
PROVIDER_VERSION be passed (make verify-ocp-manifests depend on a new prereq
that errors if PROVIDER_VERSION is empty or equals the value extracted from the
file), so the target cannot be run locally without an explicit upstream
PROVIDER_VERSION; implement one of these checks at the top of the target and
emit a clear error message guiding the caller to run
PROVIDER_VERSION=UPSTREAM_RELEASE_VERSION make ocp-manifests or enable CI.
There was a problem hiding this comment.
- The current solution makes it possible to run locally.
- The correct PROVIDER_VERSION is not available. It is based on a GitHub release and not stored anywhere in the repository.
My solution to restore manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml after diff is found to prevents the regeneration with old release version.
Yes, developers can make ocp-manifests with old version and it would pass. But we have code review and automation that runs this make target with the correct version.
There was a problem hiding this comment.
@RadekManak Thank you for the clarification! I understand your design rationale now:
- Local execution support - Developers can verify locally without needing to know the upstream version
- Version sourcing - The correct PROVIDER_VERSION comes from GitHub releases and isn't tracked in the repo
- Safeguards in place - The manifest restoration prevents accidental commits, and CI + code review ensure correctness when it matters
Your approach makes sense given these constraints. The NOTE comment already documents the limitation about version verification, and your multi-layered safeguards (restore + CI + code review) provide reasonable protection against incorrect updates.
I'll withdraw my concern about the design—it's a pragmatic solution for the problem space. ✅
|
/test unit |
openshift/Makefile
Outdated
There was a problem hiding this comment.
Here we are assuming yq is available. I don't think it will be in CI.
I'd say we could define something like so
YQ := go run github.com/mikefarah/yq/v4@v4.48.1
# then use it
$(YQ) eval ...
96a0553 to
f5950c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openshift/Makefile (2)
25-25: Extract the hardcoded manifest path to a variable for maintainability.The path
manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yamlappears three times (line 25, and twice in line 28). Extract it to a variable at the top of the target to reduce duplication and make future updates easier.verify-ocp-manifests: + $(eval MANIFEST_FILE := manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml) @echo "Extracting provider version from existing manifest..." - $(eval PROVIDER_VERSION := $(shell ${YQ} eval '.metadata.labels."provider.cluster.x-k8s.io/version"' manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml)) + $(eval PROVIDER_VERSION := $(shell ${YQ} eval '.metadata.labels."provider.cluster.x-k8s.io/version"' $(MANIFEST_FILE))) @echo "Using PROVIDER_VERSION=$(PROVIDER_VERSION) for verification" @$(MAKE) ocp-manifests PROVIDER_VERSION=$(PROVIDER_VERSION) - @./verify-diff.sh && (git restore manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml; echo -e "\nVerification successful! Manifests are in sync.") || (git restore manifests/0000_30_cluster-api_04_cm.infrastructure-aws.yaml; echo -e "\nVerification failed! Manifests are out of sync.\nPlease run:\n PROVIDER_VERSION=<release-version> make ocp-manifests\n\nWhere <release-version> is the desired upstream release version tag (e.g., v2.9.2) that matches the upstream version the OpenShift fork is based on.\n"; exit 1) + @./verify-diff.sh && (git restore $(MANIFEST_FILE); echo -e "\nVerification successful! Manifests are in sync.") || (git restore $(MANIFEST_FILE); echo -e "\nVerification failed! Manifests are out of sync.\nPlease run:\n PROVIDER_VERSION=<release-version> make ocp-manifests\n\nWhere <release-version> is the desired upstream release version tag (e.g., v2.9.2) that matches the upstream version the OpenShift fork is based on.\n"; exit 1)Also applies to: 28-28
28-28: Refactor the complex one-liner for readability and maintainability.Line 28 combines multiple concerns (diff check, restore, success/failure messaging) into a single complex shell expression with nested conditionals. This makes it difficult to understand, debug, and maintain. Consider breaking it into separate recipe lines using intermediate targets or a helper script.
Example refactor structure:
verify-ocp-manifests: $(MANIFEST_FILE) @echo "Extracting provider version from existing manifest..." $(eval PROVIDER_VERSION := $(shell ${YQ} eval '.metadata.labels."provider.cluster.x-k8s.io/version"' $(MANIFEST_FILE))) @echo "Using PROVIDER_VERSION=$(PROVIDER_VERSION) for verification" @$(MAKE) ocp-manifests PROVIDER_VERSION=$(PROVIDER_VERSION) @if ./verify-diff.sh; then \ git restore $(MANIFEST_FILE); \ echo -e "\nVerification successful! Manifests are in sync."; \ else \ git restore $(MANIFEST_FILE); \ echo -e "\nVerification failed! Manifests are out of sync.\n..."; \ exit 1; \ fiAlternatively, move the verification logic into a helper shell script for even greater clarity.
📜 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 (2)
openshift/Makefile(1 hunks)openshift/verify-diff.sh(1 hunks)
🧰 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:
openshift/verify-diff.shopenshift/Makefile
🔇 Additional comments (2)
openshift/verify-diff.sh (1)
1-11: Script logic is sound and properly handles quoting.The untracked file check is clean, error messages are clear, and the exit code propagation from
git diff --exit-codeis correct. TheFILE_DIFFvariable is properly quoted on line 7, addressing the earlier concern.openshift/Makefile (1)
28-28: Verify path resolution for./verify-diff.shacross invocation contexts.The relative path
./verify-diff.shon line 28 assumes the working directory is the openshift/ directory. If this Makefile is invoked from the repository root (e.g., via a parent Makefile ormake -f openshift/Makefile), the path will not resolve correctly.Verify the intended invocation pattern and, if needed, use a robust path relative to the Makefile location. For example:
SCRIPT_DIR := $(dir $(MAKEFILE_LIST)) @$(SCRIPT_DIR)verify-diff.shCan you confirm how this Makefile is typically invoked in the repository? Is it always run with
make -C openshift, or can it also be invoked from the root withmake -f openshift/Makefile?
RadekManak
force-pushed
the
verify-ocp-manifests
branch
from
f5950c3 to
ca93823
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (2)
openshift/Makefile(1 hunks)openshift/verify-diff.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- openshift/verify-diff.sh
🧰 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:
openshift/Makefile
🔇 Additional comments (1)
openshift/Makefile (1)
8-8: YQ tool availability is well-handled.Using
go run github.com/mikefarah/yq/v4@v4.48.1ensures yq is available without requiring pre-installation—this addresses the prior concern raised by damdo and is a pragmatic choice.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
|
@RadekManak: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/assign @mdbooth |
5 participants
This PR adds make target that verifies ocp-manfiests does not produce a diff in the transport configmap.
It is taking the PROVIDER_VERSION from the generated file. Due to that, it won't be able to verify whether the version was updated or not.