-
Notifications
You must be signed in to change notification settings - Fork 16
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
Verify previous version only when it is needed #127
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7 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 |
f803520
to
071a91a
Compare
Skip CI on VERSION change and enforce when PREVIOUS_VERSION equals VERSION. To skip CI we set DUMMY_VERSION in post-submit to CI_VERSION
071a91a
to
5e38ab3
Compare
Simplify skip verification in an easier way
If you don't provide PREVIOUS_VERSION to CI when |
@@ -34,7 +34,7 @@ jobs: | |||
- name: Build and push versioned CSV and images for tags | |||
if: ${{ github.ref_type == 'tag' }} | |||
# remove leading 'v' from tag! | |||
run: export VERSION=$(echo $GITHUB_REF_NAME | sed 's/v//') && make container-build-and-push-community | |||
run: export VERSION=$(echo $GITHUB_REF_NAME | sed 's/v//') && export SKIP_VERIFY_PREVIOUS_VERSION="true" && make container-build-and-push-community |
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.
when you want to skip verification for community releases, why not doing that in the Makefile, instead of having to set new env vars when calling the community target?
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.
when you want to skip verification for community releases, why not doing that in the Makefile, instead of having to set new env vars when calling the community target?
Your intuition is partly correct.
I do want to skip verification for community releases but only on CI for the bundle image. Thus, I don't want to skip that in the makefile (in general), as I still want the option of running this target locally without skipping the verification.
Soon when there will be automation for creating the bundle images of communities and submitting the PRs, then we might want to revisit the option of local testing with the verification for PREVIOUS_VERSION.
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.
for community releases but only on CI for the bundle image
I don't understand. Running make container-build-and-push-community
with VERSION
set is a doing a community release, no matter if running locally or CI 🤔
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.
I will elaborate on how I understand this issue, but in general, when I used "locally" I meant using CLI prior to the creation of PRs submission to the community repos.
The community bundle creation includes some modifications to the bundle CSV where one of them is updating the replaces field for defining from which versions we can update to the version of the CSV. In #108 we introduced this field with a check to the PREVIOUS_VERSION
variable so that the replaces field would have a proper and meaningful value.
IMHO we need this verification in NMO only before we create the PRs for the community repos* (OKD and OperatorHub), and it can be skipped in other places (e.g., at Github with post-PR for pushing to Quay or pre-PR for PR validity).
In the current usage of CI, we would like to skip CI testing to avoid failing it on the "bad" assignment of the PREVIOUS_VERSION
variable. This part can be improved so there won't be "bad" assignments. But I would prefer to deal with that afterward.
*Eventually we realized that OperatorHub CI is complaining about this field... So it is only needed for the RH community (/OKD)
verify-previous-version: ## Verifies that PREVIOUS_VERSION variable is set | ||
@if [ $(VERSION) != $(DEFAULT_VERSION) ] && [ $(VERSION) != $(CI_VERSION) ] && [ $(PREVIOUS_VERSION) = $(DEFAULT_VERSION) ]; then \ | ||
verify-previous-version: ## Verifies that PREVIOUS_VERSION variable is set properly (but not in CI) | ||
@if [[ ($(PREVIOUS_VERSION) != $(DEFAULT_VERSION) && $(PREVIOUS_VERSION) == $(VERSION)) ||\ |
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.
when you skip verification of the PREVIOUS_VERISION, you still add the replaces field. With the DEFAULT_VERSION 0.0.1. Does that make sense?
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, I was oblivious to that.
When running the bundle-update on CI then verify-previous-version
won't "fail", so the next "steps" in bundle-update are proceeded (including setting the replaces field).
There was some confusion about whether the validation was needed. We do want it and on pushing tags to GitHub we would like to use a manual workflow that will have a proper |
Why we need this PR
Fix a problem that causes pushing tags to fail and pushing git tags to Quay.
Changes made
Now the target makes sure to fail on a bad PREVIOUS_VERSION value
Which issue(s) this PR fixes
An issue with the
verify-previous-version
target that wasn't fixed properly in #124