-
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
use reusable workflows in create-release workflow. #454
use reusable workflows in create-release workflow. #454
Conversation
.github/workflows/create-release.yml
Outdated
version: ${{ steps.gen-version.outputs.VERSION }} | ||
gen-version: | ||
name: Generate semantic version from branch and tags | ||
uses: kyma-project/eventing-tools/.github/workflows/get-version-from-release-branch-reusable.yml@main |
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.
Will this always return the workflow yaml from main~head
? If yes, can we use a commit SHA instead, just in case main~head
gets broken for some reason.
Note: This also applies to other places in this PR using
@main
.
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.
Well, I am happy that you ask about this because I was also between these two options.
Sticking this to a commit sha makes this less susceptible to bug, so yes, let's do that.
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 changed it to a commit sha everywhere.
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 need to update the create-draft-release
workflow sha, after this PR got merged.
.github/workflows/create-release.yml
Outdated
with: | ||
fetch-depth: 0 | ||
bump-sec-scanners-config: | ||
name: Bump the sec-scandners-config.yaml |
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.
name: Bump the sec-scandners-config.yaml | |
name: Bump the sec-scanners-config.yaml |
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.
fixed!
.github/workflows/create-release.yml
Outdated
bump-sec-scanners-config: | ||
name: Bump the sec-scandners-config.yaml | ||
needs: gen-version | ||
uses: kyma-project/eventing-tools/.github/workflows/bump-sec-scanners-config-reusable.yml@main |
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.
(nit) ignore for now, after noticing how the reusable workflows are used, maybe it would have been better to use the following naming instead of the current one:
kyma-project/eventing-tools/.github/workflows/reusable/bump-sec-scanners-config.yaml
but this is only my personal preference, so feel free to ignore.
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.
That might have been a helpful solution. You would still find workflows at the expected place and the extra dir would make clear what this is, especially when we are calling them from other workflow like in this case. Learnings for the future.
echo "VERSION=${VERSION}" >> $GITHUB_OUTPUT | ||
run-unit-test: | ||
name: Run Unit Tests | ||
needs: [gen-version, bump-sec-scanners-config] |
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.
Why does run-unit-test
need bump-sec-scanners-config
?
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.
It does not need it, it will just wait to run the unit tests after bump-sec-scanners-config
(because of the PR we create in the that part). In general, all the jobs will run consecutively. Only lint
and unit-test
will run in parallel because they are independent of each other. That flow is controlled by the need
instructions.
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.
the only flow that is really needed is the gen-version
because only if it get's marked as needs
the output (VERSION
) gets available in the other jobs.
.github/workflows/create-release.yml
Outdated
uses: kyma-project/eventing-tools/.github/workflows/render-and-upload-manifests-reusable.yml@main | ||
with: | ||
VERSION: ${{ needs.gen-version.outputs.VERSION }} | ||
CR_FILE: "somedir/cr_file.yaml" |
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.
somedir
?
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.
Oh , I forgot to change the names from tests that I have done earlier. Thanks. I fixed it.
But while I fixed this, I noticed that I have done a mistake when I implemented the flow and only tested it with "somedir/cr_file.yaml".
We need a way to rename the default cr, since gh cannot do it or upload it with a different name.
Here is the fix: would you also take a look at this? I will update the commit sha later, once it was merged.
https://github.com/kyma-project/eventing-tools/pull/66/files
@@ -0,0 +1,57 @@ | |||
#!/usr/bin/env bash |
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.
Does it make sense to move this script to the common tools repo so other repositories may use it?
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 thought about this. My intention was to give more flexibility with the changelog (similar to the sec-scanners-config
and render-manifest
). But you are right, in this case it is not needed at all. I added this PR to do so.
done | ||
|
||
# Create a new file (with a unique name based on the process ID of the current shell). | ||
NEW_CONTRIB=$$.new |
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.
(nit) $$.new
-> $$.authors
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.
done
echo -e "\n**Full changelog**: https://github.com/$REPOSITORY/compare/${PREVIOUS_RELEASE}...${RELEASE_TAG}" >>${CHANGELOG_FILE} | ||
|
||
# Cleanup the NEW_CONTRIB file. | ||
rm ${NEW_CONTRIB} || echo "cleaned up" |
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.
(nit) IMO, no need to echo anything here, because removing the file is an internal housekeeping.
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 removed the whole section. It is just not needed.
echo "fetching webhook image from ${WEBHOOK_FILE}" | ||
WEBHOOK_IMAGE=$(yq eval '.images[0].newName' <"$WEBHOOK_FILE") | ||
WEBHOOK_TAG=$(yq eval '.images[0].newTag' <"$WEBHOOK_FILE") | ||
echo -e "webhook image is ${WEBHOOK_IMAGE}:${WEBHOOK_TAG} \n" |
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.
(nit) Maybe set the var WEBHOOK_IMAGE
to be ${WEBHOOK_IMAGE}:${WEBHOOK_TAG}
.
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 like the idea. Done.
- ${PUBLISHER_IMAGE} | ||
- ${WEBHOOK_IMAGE}:${WEBHOOK_TAG} |
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.
After applying this comment, these lines should read:
- ${PUBLISHER_IMAGE}
- ${WEBHOOK_IMAGE}
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.
Updated.
Co-authored-by: Marco Bebway <[email protected]>
Co-authored-by: Marco Bebway <[email protected]>
I will set this to |
now useschanges from kyma-project/eventing-tools#68
Co-authored-by: kyma-eventing-bot <kyma-eventing-bot"@users.noreply.github.com>
8968d81
to
3720d1f
Compare
e94291a
to
0b0d1e1
Compare
@friedrichwilken: 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/test-infra repository. I understand the commands that are listed here. |
Description
Changes proposed in this pull request:
Create release
workflowCreate release
workflowsec-scanners-config.yaml
Resons for the removal of some files:
scripts/check_release_tag.sh
was used increate-release
to verify theversion
the user provided via input. Since we now derive the version from the branch name, this is no longer needed ( it is done here instead).scripts/check_sec-scanners-config.sh
was used in thecreate-release
workflow to verify if the release version is the version used in thesec-scanners-config.yaml
as the eventing-manager image tag. Since we now render thesec-scanners-config.yaml
via bump-sec-scanners-config, this is no longer needed.scripts/create_changelog.sh
will be moved toeventing-tools
by this PRscripts/create_draft_release.sh
will be replaced bycreate-draft-release-reusable.yml
scripts/publish_release.sh
will be replaced bypublish-release-reusable.yml
scripts/render_and_upload_manifests.sh
will be replace byrender-and-uploader-manifests-reusable.yml
scripts/verify-status.sh
was used to wait the prow build job to finish successfully. This will happen intrigger-prow-build-job-reusable.yml
by usingkyma-project/wait-for-commit-status-action
.Related issue(s)
#361