Fix Docker tag logic to prevent undefined in dev tag #3524
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will make the transition simple when we decide that removing a node suffix will never be necessary.
godber
approved these changes
Jan 12, 2024
godber
reviewed
Jan 16, 2024
| }); | ||
|
|
||
| it('should not modify a tag without -nodev', async () => { | ||
| const tagBefore = 'v0.91.0-v18.18.2'; |
There was a problem hiding this comment.
Is this an actual case that would be encountered? v0.91.0-v18.18.2
If not what are more realistic cases that could be tested.
There was a problem hiding this comment.
I could test the daily tags: daily-2024.01.12-fe31c87bf9f and daily-2024.01.12-fe31c87bf9f-nodev16.20.2
This would test a tag with a hyphen in it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes the following changes:
getDevDockerImage- this fixes the undefined, but results in the node version being added to the tag twice.removeNodeSuffixFromTagfunction. This turns something likev0.91.0-nodev18.18.2intov0.91.0.options.nodeSuffixand removes the tag iffalse.I chose to add the node suffix by default to make the transition simple when we decide that having a tag without a node suffix will never be necessary. At that point we can update
formatDailyTag()to takenodeVersionas a parameter and update its test.