-
Notifications
You must be signed in to change notification settings - Fork 14
Remove auto increment of ruby version number for dev runs #5799
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
Conversation
|
|
tests/test_the_test/test_version.py
Outdated
| assert ComponentVersion("ruby", "1.0.0.rc1") < "[email protected]" | ||
|
|
||
| assert ComponentVersion("ruby", "2.3.0 7dbcc40") >= "[email protected].1-dev" | ||
| assert ComponentVersion("ruby", "2.3.0 7dbcc40") >= "[email protected].0-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.
All of these comparisons are hard to read. Are they backwards? In the (system tests) code the version specified is the required minimum one and it's compared against the library version. Do these tests have the library version on the left and the minimum target on the right? If so why?
Assuming I understood the test correctly, this particular one seems wrong: if the specification in ST is 2.3.0-dev, the test should run against library version "2.3.0 7d...", although we don't ever have versions like that so this test is also confusing as to what exactly it's meant to exercise.
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 believe this test is meant to test the version transformation logic from ComponentVersion and not the end to end test activation process. So the order of the expressions is arbitrary.
This particular test has the result you expect it to have. Since 2.3.0 7dbcc40 >= 2.3.0-dev if you declare 2.3.0-dev in the manifest the test would run for 2.3.0 7dbcc40.
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.
OK but why would I use 2.3.0-dev in the manifest? I would expect to simply use 2.3.0? Is there documentation for how the manifest versions work?
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 would use 2.3.0-dev if you are working on version a dev version for 2.3.0 and you want the test to run on this dev version. But the version management is owned by the library team so if you want to do things differently you can. This is just supported (as per the semver spec) for all languages.
|
I don't see the most important test: given version spec in ST of 2.23.0, and current library version of 2.23.0.dev (assuming this is what we actually generate, I'm not sure this is properly happening at the moment), the test should run. |
|
The test you are talking about is made in two parts, we check that the translation is done correctly and then that the comparison works as intended with the translated value. That said I added the test you are talking about for completeness. |
|
Per the slack discussion, it looks like dd-trace-rb has https://github.com/DataDog/dd-trace-rb/blob/895d80eb07eea0689769d7170f1c9d3ca339a97b/.gitlab/patch_gem_version.sh#L33-L34 which is supposed to be invoked during gem building to make the version number of the library correct. Is this being called by ST somewhere? I don't see any references to this file in ST repo. Without this patch invoked the library version of dd-trace-rb won't be correct (e.g. it will be 2.23.0 instead of 2.23.0.dev during development), and subsequent comparisons won't work right. I still think dd-trace-rb should have the .dev suffix in the version spec without needing to patch it in as a manual step. |
|
I couldn't find any reference to this script in ST either but the version in the ci of this PR does contain the -dev tag (which is derived from .dev). More over on the ST side of things what really matters is that the version is bumped after a release. If the dev tag is missing it shouldn't have an impact. |
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 think the non-test changes here are desirable and, perhaps, a prerequisite for DataDog/dd-trace-rb#5108 to be merged.
The test changes are still confusing to me but I have no objections to them, if you think they are correct I approve.
The tests do not contain a Ruby version of form 2.23.0.dev which will be in place after DataDog/dd-trace-rb#5108 is merged, but is (should?) not currently be appearing anywhere in ST runs given that ST is not invoking the version patching script used in gitlab CI tasks.
I think this PR should be merged and then the tests in DataDog/dd-trace-rb#5108 rerun to see if they would then pass.
Motivation
Changes
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>], double-check that only<language>is impacted by the changebuild-XXX-imagelabel is present