Skip to content
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

Allow canary releases #605

Closed
wants to merge 16 commits into from
Closed

Allow canary releases #605

wants to merge 16 commits into from

Conversation

dsame
Copy link
Contributor

@dsame dsame commented Oct 24, 2022

Description:
https://github.com/actions/runner-images-internal/issues/4457

Related issue:
Add link to the related issue.

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@dsame dsame requested a review from a team October 24, 2022 07:22

return version;
}

// TODO - should we just export this from @actions/tool-cache? Lifted directly from there
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment isn't relevant anymore since we're changing the logic

Comment on lines +391 to +409
versions = versions.sort((a, b) => {
if (semver.gt(a, b)) {
return 1;
}
return -1;
});
Copy link
Contributor

@brcrista brcrista Oct 25, 2022

Choose a reason for hiding this comment

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

There's a compare function you can pass to Array.prototype.sort: https://github.com/npm/node-semver#comparison

Suggested change
versions = versions.sort((a, b) => {
if (semver.gt(a, b)) {
return 1;
}
return -1;
});
versions = versions.sort(semver.compare);

Comment on lines +438 to +458
const versionsReversed = versions.sort((a, b) => {
if (semver.gt(a, b)) {
return -1;
} else if (semver.lt(a, b)) {
return 1;
}
return 0;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also an rcompare function:

Suggested change
const versionsReversed = versions.sort((a, b) => {
if (semver.gt(a, b)) {
return -1;
} else if (semver.lt(a, b)) {
return 1;
}
return 0;
});
const versionsReversed = versions.sort(semver.rcompare);

versionSpec: string
): string {
let version = '';
let range: string | null | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let range: string | null | undefined;
let range: string | undefined;

doesn't look like it's ever null?

Copy link
Contributor

@brcrista brcrista left a comment

Choose a reason for hiding this comment

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

A few minor suggestions but looks good

versionSpec: string
): string {
let version = '';
let range: string | null | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let range: string | null | undefined;
let range: string | undefined;

doesn't look like it's ever null?

@dsame dsame force-pushed the v-sedoli/lkgr branch 6 times, most recently from 206b7e5 to 8285caf Compare November 2, 2022 20:50
@dsame dsame force-pushed the v-sedoli/lkgr branch 6 times, most recently from 76c3871 to 21dbe7e Compare November 3, 2022 20:00
@dsame
Copy link
Contributor Author

dsame commented Nov 11, 2022

The PR to be abandoned for sake of this PR #619

@dmitry-shibanov
Copy link
Contributor

I'm going to close the pull request in favour of this: #655

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants