-
Notifications
You must be signed in to change notification settings - Fork 22
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
buildx: make it easier to download precompiled binaries from other repos #136
Conversation
this is a re-roll of #127, but integrates better with the htc changes. i'm still not 100% sure how we should expose this in setup-buildx |
src/buildx/install.ts
Outdated
public async download(version: string): Promise<string> { | ||
const release: GitHubRelease = await Install.getRelease(version); | ||
core.debug(`Install.download release tag name: ${release.tag_name}`); | ||
public async download(version: string, owner?: string, repo?: string): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to change the signature imo. As said in previous PR we can just set version
to a tagged release URL like https://github.com/docker/buildx/releases/tag/v0.11.1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! i changed this to accept a url of that form
src/buildx/install.ts
Outdated
|
||
const release = await this.getReleaseMetadata(version, realOwner, realRepo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not always rely on the API. We got many issues in the past doing this for GHES users: docker/setup-buildx-action#194 and also the API being broken: docker/setup-buildx-action#190. That's why we rely on the assets CDN instead.
So looking at my previous comment I think we should call getReleaseMetadata
only if a tagged release URL is detected otherwise we rely on getRelease
as before if semver (will download from vanilla repo docker/buildx).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with this I think we should rename getRelease
to getReleaseFromJSON
and getReleaseMetadata
to getReleaseFromAPI
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm, done
8297957
to
cc39d3c
Compare
actually, one sec, i'm going to update it to also support releases/latest |
After this change, callers can pass a release URL of the form: https://github.com/docker/buildx/releases/tag/v0.11.1 and get the release binaries from the the github api. Signed-off-by: Nick Santos <nick.santos@docker.com>
cc39d3c
to
2e3bbcf
Compare
ok, updated! ✔️ |
chatted off-list, we're going to use the approach here - 20e2f43 |
After this change, callers can pass a release URL of the form:
https://github.com/docker/buildx/releases/tag/v0.11.1
and get the release binaries from the the github api