-
Notifications
You must be signed in to change notification settings - Fork 47
NO-JIRA: Add manifests verify target #579
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?
Conversation
|
@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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
WalkthroughAdded two verification targets to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
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: 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)
686cbdd to
5ccde04
Compare
|
@RadekManak: The following tests 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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.