-
Notifications
You must be signed in to change notification settings - Fork 398
Adjust gem version management to have .dev version in master #5108
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: master
Are you sure you want to change the base?
Conversation
|
|
This PR is failing system tests e.g. https://github.com/DataDog/dd-trace-rb/actions/runs/19868421868/job/56937408519?pr=5108 which expect the hacked version, maybe it needs to be merged after DataDog/system-tests#5799 or some other coordination is required. |
ivoanjo
left a 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.
👍 We've slowly been moving in the direction of doing this, so I think it's time we rip-off the band-aids, and just fix whatever CI/tests need to be updated to match this change.
| version: | ||
| description: Version to update to | ||
| description: Version to update to; should usually have .dev suffix e.g. 2.23.0.dev | ||
| required: true | ||
| type: string | ||
| workflow_call: | ||
| inputs: | ||
| version: | ||
| description: Version to update to | ||
| description: Version to update to; should usually have .dev suffix e.g. 2.23.0.dev | ||
| required: true | ||
| type: string |
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.
Minor: Rather than an indication, should we make the rake task actually fail if it's not .dev?
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 don't know if this would be OK...
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.
If it turns out to not be great, we can always undo it -- what's the worst thing that can happen if a rake task used to bump the version refuses to bump 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.
(You can say it was my fault 🤣 )
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.
This comment is on a GH workflow change but you suggest changing the rake task. The rake task is used during release process where it's getting the version-to-be-released as the argument (e.g. 2.23.0), so that definitely cannot be constrained to dev only. Did you mean the rake task or the GH workflow restriction? For GH workflow I'd have to research how to 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.
Actually I thought the release bump was triggered from https://github.com/DataDog/fast_castle/blob/main/dd-trace-rb/steps.rb but I don't see where now, maybe I'm wrong on that too.
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.
Actually, it wasn't needed before, but we may need to adjust that. If you think about it, before, there was only 1 thing needed: previous version -> next version. Master would be all set for release.
Now due to this PR we'll have previous version -> next dev. We'll need to change fast_castle (or something else) to have next dev -> next version.
What does this PR do?
Adjusts the code and scripts to have the gem version in master be suffixed with
.devduring development._ Alters lib/datadog/version.rb to specify
.devsuffix. This should be the "steady state" unless we are actively releasing.This change means that when anyone builds the gem from master, they would get a version number like
2.23.0.dev. Currently the version would be2.23.0which is wrong - we haven't released 2.23.0 yet. The prematurity of release version appearing in our sources is, for example, causing the current issues/confusion with system tests._ Adds error checking and a diagnostic report to
dd-trace-rb/.gitlab/patch_gem_version.sh
Lines 33 to 34 in 895d80e
.devsuffix as far as I can tell._ As far as I can tell no action is needed to go from
2.23.0.devto2.23.0during release process. The version to release is specified manually and it's already2.23.0, this will require no changes. I verified that the rake task used to bump the version works with the existing tree having the.devsuffix. This is called fromdd-trace-rb/.github/workflows/bump-gem-version.yml
Line 51 in 895d80e
_ The post-release version bump is confusing. I found https://github.com/DataDog/fast_castle/blob/main/dd-trace-rb/steps.rb#L389 but this does not create the pull request. I can't find where the PR is actually generated.
I adjusted
dd-trace-rb/.github/workflows/bump-gem-version.yml
Line 7 in 895d80e
.devsuffix, because the only time they wouldn't should (I think) be handled by the release process, as specified above.Motivation:
Review of DataDog/system-tests#5799 + I think ST does not run our tests properly or I don't understand the versioning
Change log entry
None
Additional Notes:
The ST hackage was implemented in DataDog/system-tests#3022, it should not be needed with the changes in this PR (but possibly ST needs to be updated to deal with the Ruby version format of
2.23.0.devinstead of2.23.0-devthat is common elsewhere).How to test the change?
I tested all of the above mentioned scripts locally with various inputs