🌱 improve the verify-crd-compatibility make target #1499
Conversation
for use during local development by making the default updated source be the local file, picking up changes not yet committed. The previous git sourcing required changes to be committed to be picked up and is not a very good developer experience as you should validate the changes prior to commiting them. Since the git source is necessary for CI, the GitHub Action is updated to specify the git source for the updated file. Signed-off-by: everettraven <[email protected]>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -132,10 +132,10 @@ bingo-upgrade: $(BINGO) #EXHELP Upgrade tools | |||
|
|
|||
| .PHONY: verify-crd-compatibility | |||
| CRD_DIFF_ORIGINAL_REF := main | |||
| CRD_DIFF_UPDATED_REF := HEAD | |||
| CRD_DIFF_UPDATED_SOURCE := file://config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml | |||
There was a problem hiding this comment.
I'm assuming this isn't treated as an absolute path?
There was a problem hiding this comment.
Nope. It will read it as a relative path.
| CRD_DIFF_CONFIG := crd-diff-config.yaml | ||
| verify-crd-compatibility: $(CRD_DIFF) | ||
| $(CRD_DIFF) --config="${CRD_DIFF_CONFIG}" "git://${CRD_DIFF_ORIGINAL_REF}?path=config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml" "git://${CRD_DIFF_UPDATED_REF}?path=config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml" | ||
| verify-crd-compatibility: $(CRD_DIFF) manifests |
There was a problem hiding this comment.
Adding manifests to this target will cause it to also be run in the CI, and if there's a diff, it won't be caught by the GitHub workflow, because it's looking directly at SHAs.
That being said, this should fail make verify
There was a problem hiding this comment.
Correct. In CI, the expectation is that it is looking at the committed changes. I would push for us to rely on the distinct checks for "uncommitted, generated, changes" in CI.
|
⛔ This should be held, unless we consider it a bug. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/lgtm |
perdasilva
removed
the
do-not-merge/hold
|
unholding as v1 has been released 🎉 |
for use during local development by making the default updated source be the local file, picking up changes not yet committed. The previous git sourcing required changes to be committed to be picked up and is not a very good developer experience as you should validate the changes prior to commiting them. Since the git source is necessary for CI, the GitHub Action is updated to specify the git source for the updated file.
Reviewer Checklist