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

feat: download and test with the latest release #6638

Closed
wants to merge 7 commits into from

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Jan 21, 2025

Still a work in progress because this should test that it will upgrade to the new package. Right now it is just running an e2e test against the latest release for the relevant channel.

I'm just opening this draft early to get some feedback.

@lerouxb lerouxb added the wip label Jan 21, 2025
@github-actions github-actions bot added the feat label Jan 21, 2025
| 'linux_deb'
| 'linux_rpm';

function getPlatformShortName(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The URL for downloading the latest release ends in things like osx, darwin-arm64, darwin-x64, windows, linux_dev or linux_rpm. This just calculates that bit. I'm calling it a "platform short name" until we think of something better.

});

try {
runTest({ appName, appPath });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again: This should actually see if you can update from this version. I'm just including it as a kind of sanity check that releasepath works.


export function assertCommonBuildInfo(
buildInfo: unknown
): asserts buildInfo is CommonBuildInfo {
assertObjectHasKeys(buildInfo, 'buildInfo', commonKeys);
assert(
SUPPORTED_CHANNELS.includes((buildInfo as any).channel),
Copy link
Contributor

@kraenhansen kraenhansen Jan 21, 2025

Choose a reason for hiding this comment

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

This is probably the easiest. It's a bit annoying that Array#includes doesn't simply take an unknown, but .. there are reasons.

Suggested change
SUPPORTED_CHANNELS.includes((buildInfo as any).channel),
SUPPORTED_CHANNELS.includes((buildInfo as unknown as Channel).channel),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buildInfo isn't a channel, though. it has a channel. And it is already unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like:

export function assertCommonBuildInfo(
  buildInfo: unknown
): asserts buildInfo is CommonBuildInfo {
  assertObjectHasKeys(buildInfo, 'buildInfo', commonKeys);
  assert(
    SUPPORTED_CHANNELS.includes((buildInfo as { channel: Channel }).channel),
    `Expected ${
      (buildInfo as { channel: Channel }).channel
    } to be in ${SUPPORTED_CHANNELS.join(',')}`
  );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use assertObjectHasKeys to make sure buildInfo has the channel property? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already did. I added channel to commonKeys.

Copy link
Contributor

@kraenhansen kraenhansen Jan 25, 2025

Choose a reason for hiding this comment

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

Then why the (buildInfo as { channel: Channel }).channel? 🤔
Could that be buildInfo.channel as Channel instead?

@lerouxb lerouxb added the no release notes Fix or feature not for release notes label Jan 22, 2025
@lerouxb lerouxb closed this Jan 30, 2025
@lerouxb lerouxb deleted the download-released-compass branch January 30, 2025 20:59
@lerouxb
Copy link
Contributor Author

lerouxb commented Jan 30, 2025

Closing in favour of #6669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes wip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants