Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

allow :latest when specifying image tag #627

Closed
wants to merge 5 commits into from
Closed

Conversation

agorskiy12
Copy link
Contributor

allow to use :latest when specifying image tag

Provide a general summary of your changes in the Title above.

Description

:latest should be allowed when specifying image tag

Types of changes

Check boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update
  • Security update

Checklist

Check boxes that apply:

  • My code follows the code style of this project.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • I have added the Apache 2.0 license header to any new files created.

":latest" image tag
@@ -138,7 +138,7 @@ const generateDebug = (port, useDebug) => (useDebug ? `--inspect=0.0.0.0:${port}
// So we have to remove those flags if the one-app version is less than 5.13.0
// 5.13.0 is when node 16 was introduced.
const generateNodeFlags = (appVersion) => {
if (semver.intersects(appVersion, '^5.13.0', { includePrerelease: true })) {
if (appVersion === 'latest' ? true : semver.intersects(appVersion, '^5.13.0', { includePrerelease: true })) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (appVersion === 'latest' ? true : semver.intersects(appVersion, '^5.13.0', { includePrerelease: true })) {
if (appVersion === 'latest' || semver.intersects(appVersion, '^5.13.0', { includePrerelease: true })) {

We could avoid the ternary, but its not a blocker

Copy link
Contributor Author

@agorskiy12 agorskiy12 Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, now that I think about it, the reason I added ternary was because I kept getting this error with the or operator:

Invalid comparator: latest ,

it seems that semver.intersects doesnt like "latest". So, reverting back to my original code that avoids semver when latest is specified.

image

@Matthew-Mallimo
Copy link
Member

@agorskiy12 Can you also test for when :6 is provided? @smackfu had mentioned that its failing a check for that as well. We should be adding the flags for one-app versions >=6.6.0

@agorskiy12
Copy link
Contributor Author

@agorskiy12 Can you also test for when :6 is provided? @smackfu had mentioned that its failing a check for that as well. We should be adding the flags for one-app versions >=6.6.0

OK I'll check for that.

are you saying we should ONLY be adding flags for versions >=6.6.0 ? Could you elaborate why? right now we're allowing anything after 5.13.0

@Matthew-Mallimo
Copy link
Member

Matthew-Mallimo commented Mar 25, 2024

@agorskiy12 Can you also test for when :6 is provided? @smackfu had mentioned that its failing a check for that as well. We should be adding the flags for one-app versions >=6.6.0

OK I'll check for that.

are you saying we should ONLY be adding flags for versions >=6.6.0 ? Could you elaborate why? right now we're allowing anything after 5.13.0

>=5.13.0 and >=6.6.0 are on Node 18, where these flags are needed. Anything prior to those versions are on Node 16 where these flags dont exist

@agorskiy12
Copy link
Contributor Author

@agorskiy12 Can you also test for when :6 is provided? @smackfu had mentioned that its failing a check for that as well. We should be adding the flags for one-app versions >=6.6.0

OK I'll check for that.
are you saying we should ONLY be adding flags for versions >=6.6.0 ? Could you elaborate why? right now we're allowing anything after 5.13.0

>=5.13.0 and >=6.6.0 are on Node 18, where these flags are needed. Anything prior to those versions are on Node 16 where these flags dont exist

OK i see. I also see why something like 6.6.0 wouldnt work -

caret ^ means any version compatible, but anything with 6 will not "intersect" because theres a major version change from 5.

we will have to use something like semver.gte (greater than).

@agorskiy12 agorskiy12 marked this pull request as draft March 26, 2024 17:36
@agorskiy12 agorskiy12 closed this Mar 26, 2024
@10xLaCroixDrinker 10xLaCroixDrinker deleted the fix/allow-latest-tag branch May 15, 2024 12:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants