Skip to content

Provide option to point ReleasesApiConsumer at nightly builds repo#3127

Merged
robertbrignull merged 9 commits intomainfrom
robertbrignull/nightly-codeql
Jan 2, 2024
Merged

Provide option to point ReleasesApiConsumer at nightly builds repo#3127
robertbrignull merged 9 commits intomainfrom
robertbrignull/nightly-codeql

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Adds in the codeQL.cli.channel config option, as a way to point the ReleasesApiConsumer class at dsp-testing/codeql-cli-nightlies instead of at github/codeql-cli-binaries. This will be useful for the CodeQL team to test nightly builds of the CodeQL CLI.

When changing the cli channel, the intended behaviour is that it will change the installed distribution to whatever is the latest release of that channel. From reading the code I believe it should already behave this way. I can't find any checks of the semver of the currently installed extension vs the new extension, so I believe it relies on the releases always going forwards and never doing something like re-releasing an old version. Therefore there's no need for us to add any tracking of where the currently-installed extension came from.

I haven't tested this yet because I'm not in a position to do that today, but I will test it tomorrow when I'm more able.

When writing this I also spotted some ways we can make the ReleasesApiConsumer cleaner and fit in more with the style of the rest of the extension, but I'll save those changes for a separate PR.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team December 12, 2023 10:49
@robertbrignull robertbrignull requested a review from a team as a code owner December 12, 2023 10:49
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Unfortunately, this doesn't quite work. There is a check that all releases satisfy the semver version range 2.x, and nightly CLI releases are tagged like codeql-bundle-20231212, which are neither semver nor do they satisfy the semver range.

If we remove this semver check for nightly builds, then I think this should work. The update check seems to be based on the release ID, and doesn't even check that a release is newer when updating.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

There is a check that all releases satisfy the semver version range 2.x, and nightly CLI releases are tagged like codeql-bundle-20231212, which are neither semver nor do they satisfy the semver range.

Good point. I agree I think we want to just remove that check when using nightly builds. We'll just take the latest nightly release regardless of its name.

The update check seems to be based on the release ID, and doesn't even check that a release is newer when updating.

That's how I understood the code too. It uses the release ID to tell if we're on the exact same release already and therefore don't need to update. If the release IDs do not match then it always upgrades regardless of semver of the current and new release.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

I've worked around the nightly releases not having semver tags, but it wasn't pretty 😢

There are probably better ways of doing this. I'm also not hugely happy with the ReleasesApiConsumer class as a whole, but I'm resisting the urge to refacting it in this PR. I plan to do a second PR after this to make ReleasesApiConsumer a bit more modern and move it to its own file.

If this code works is it acceptable?

);
const versionComparison = orderBySemver
? semver.compare(semver.parse(b.tag_name)!, semver.parse(a.tag_name)!)
: b.id - a.id;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about whether to order based on id or createdAt, but went with id because it's a number and having to parse date strings (which might be invalid) so we can compare them is a pain.

I also wondered about passing in a comparison function instead this orderBySemver boolean, but thought maybe that could wait for followup refactoring.


private usingNightlyReleases(): boolean {
return (
!this.config.ownerName &&
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I find it odd that you can set a custom owner name and repo name separately, and it'll maybe use a custom org name with a default value for repo name. I'm not sure why someone would want that, but that's what we have. Therefore we have to check if either is set, and we can't just combine our values together into one NWO.

@github github deleted a comment from F4ST3R215 Dec 20, 2023
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

General approach looks good to me, but I agree that we could refactor this code to be easier to understand and perhaps split up this file into a few separate files containing the different classes.

Comment thread extensions/ql-vscode/src/codeql-cli/distribution.ts Outdated
Comment thread extensions/ql-vscode/src/codeql-cli/distribution.ts Outdated
Comment thread extensions/ql-vscode/src/codeql-cli/distribution.ts
Comment thread extensions/ql-vscode/src/config.ts Outdated
@robertbrignull robertbrignull force-pushed the robertbrignull/nightly-codeql branch from f638605 to e565032 Compare December 20, 2023 16:26
@robertbrignull
Copy link
Copy Markdown
Contributor Author

I've pushed changes in response to @koesie10's comments and also added some tests to cover the new behaviour and hopefully fixed the existing tests.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

…/distribution.test.ts

Co-authored-by: Koen Vlaswinkel <koesie10@users.noreply.github.com>
@robertbrignull robertbrignull merged commit f5dbcc8 into main Jan 2, 2024
@robertbrignull robertbrignull deleted the robertbrignull/nightly-codeql branch January 2, 2024 12:21
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.

2 participants