-
Notifications
You must be signed in to change notification settings - Fork 1
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
New Actions And Workflow for bumping version #136
base: main
Are you sure you want to change the base?
Conversation
Could you add example documentation for setting up a repository that uses this workflow (e.g. contents of |
@@ -61,11 +61,20 @@ jobs: | |||
|
|||
- name: Bump version | |||
id: bump-version | |||
uses: bakdata/ci-templates/actions/bump-version@v1.21.2 | |||
uses: bakdata/ci-templates/actions/bump-version@feat/new-bumpversion |
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.
TODO: prebump version
- name: Commit and push changes including .bumpversion.cfg file | ||
uses: bakdata/ci-templates/actions/commit-and-push@v1.6.0 | ||
with: | ||
ref: ${{ github.event.repository.default_branch }} | ||
commit-message: "Bump version ${{ steps.bump-version.outputs.release-version }} → ${{ steps.bump-version-snapshot.outputs.release-version }}" | ||
github-username: ${{ secrets.github-username }} | ||
github-email: ${{ secrets.github-email }} | ||
github-token: ${{ secrets.github-token }} |
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.
We currently need 2x commit and push in this workflow, because we are creating the GitHub release in between, which is not ideal. As the docs of the softprops/action-gh-release action (which is part of our tag-and-release action above) mention, it would be more ideal if the GitHub release is actually created in the workflow running on the tag later with an if: startsWith(github.ref, 'refs/tags/')
condition. This way we could avoid pushing and in turn triggering CI multiple times on the defualt branch.
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 still think we shouldn't create the GitHub release in the CI running on the default branch
fe39d0c
to
5eea497
Compare
@yannick-roeder regarding #136 (comment) (cant comment for some reason) |
8e89b0d
to
892c704
Compare
@@ -57,34 +52,35 @@ jobs: | |||
uses: bakdata/ci-templates/actions/checkout@1.32.0 | |||
with: | |||
ref: ${{ github.event.repository.default_branch }} | |||
persist-credentials: false # required for pushing to protected branch later | |||
persist-credentials: true # required for pushing to protected branch later |
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 did this change? I think this would lead to the CI being logged in with default credentials and not with the bot user
git add .bumpversion.cfg | ||
git commit -m "Bump version ${{ steps.bump-version.outputs.old-version }} → ${{ steps.bump-version.outputs.release-version }}" | ||
git fetch origin | ||
git rebase --strategy-option=theirs origin/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.
Why is there a need for rebasing if we checked out right before?
- name: Create changelog | ||
id: build-changelog | ||
uses: bakdata/ci-templates/actions/changelog-generate@1.35.0 | ||
uses: bakdata/ci-templates/actions/changelog-generate@tiedemann/changelog-action |
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.
Did you test the case where we also want to have the up to date snapshot in a file on the default branch and the tag?
# config example .bumpversion.cfg: | ||
# [bumpversion] | ||
# current_version = 1.0.0 | ||
# search = version: {current_version} | ||
# replace = version: {new_version} | ||
# parse = (?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)(-(?P<release>snapshot))? | ||
# serialize = | ||
# {major}.{minor}.{patch}-{release} | ||
# {major}.{minor}.{patch} | ||
|
||
# [bumpversion:part:release] | ||
# first_value = snapshot | ||
# optional_value = release | ||
# values = | ||
# snapshot | ||
# release |
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.
Nice that there is a comment here, ideally you can also put this in the docs
# release: a.b.c-snapshot -> a.b.c | ||
# release a.b.c -> crash | ||
# major: a.b.c(-snapshot)? -> a+1.0.0-snapshot | ||
# minor: a.b.c(-snapshot)? -> a.b+1.0-snapshot | ||
# patch: a.b.c(-snapshot)? -> a.b.c+1-snapshot | ||
# TLDR: release removes the snapshot suffix |
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.
Same as above, nice comments, but please also add it in the docs
Fixes #90