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

Add ability to resolve snapshot latest date #679

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

khlopko
Copy link

@khlopko khlopko commented Aug 2, 2024

Closes issue: #673

Implements resolution of development snapshots either by explicit date or by fetched tags from swiftlang/swift repo (similar to how Swiftly does this) by setting "main-snapshot" or "X.Y-snapshot" to swift-version parameter. It relies on setting GH_TOKEN value in env from secrect.GITHUB_TOKEN to avoid hitting limits.

Unites package information building under getPackage function. Includes tests for new functionality, including snapshot resolution with cached GitHub responses. Updated README with new options for versions and adding token.

@khlopko khlopko marked this pull request as draft August 2, 2024 15:26
@khlopko khlopko force-pushed the snapshots-support branch 6 times, most recently from 57d797e to cf0fd99 Compare August 2, 2024 21:23
@khlopko
Copy link
Author

khlopko commented Aug 3, 2024

Update.

Sorry for too many pushes that created pending jobs, I didn't know that it actually creates jobs within each push, thought needs approval for this. I've figured out how to setup locally most of the stack.

As for the implementation,

  1. It works for Ubuntu runners.
  2. Will try fix for macOS today-tomorrow. Fixed.
  3. Need to check if everything is fine for release versions. Seems to work OK.

Open questions without answer as for now:

  1. Current implementation requires GitHub API token in the repo, Swiftly manages to do this without token as it seems (need to look more how it handles that), and it is not really convenient to ask adding API key. OK, they rely on free request rates by default and allow to pass token to avoid getting hit by limits.
  2. Version of the Swift for snapshot is hardcoded right now in Package, the one used to check against already installed. - this puzzles me how to address, seems like this breaks caching as well.

UPD. Unified code with what was running for stable versions, removing isStableRelease flag from the Package.

@khlopko khlopko marked this pull request as ready for review August 3, 2024 09:28
@khlopko khlopko force-pushed the snapshots-support branch 6 times, most recently from 606af3f to 318d629 Compare August 5, 2024 18:57
@vsarunas
Copy link

@fwal , have you been able to check this PR?

Copy link
Collaborator

@fwal fwal left a comment

Choose a reason for hiding this comment

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

Hello @khlopko and thank you for your PR!
This looks great, there's just some implementation details we should look at, let me know if you need any help 🙂
PS. Sorry about the slow response 😅

Comment on lines 1 to 37
export interface GitHubClient {
retryTimeout: number;
hasApiToken(): boolean;
getTags(page: number, limit: number): Promise<any>;
}

export class DefaultGitHubClient implements GitHubClient {
retryTimeout: number = 5000;
private githubToken: string | null;

constructor(githubToken: string | null = null) {
this.githubToken = githubToken || process.env.GH_TOKEN || null;
}

hasApiToken(): boolean {
return this.githubToken != null && this.githubToken != "";
}

async getTags(page: number, limit: number): Promise<any> {
const url = `https://api.github.com/repos/swiftlang/swift/tags?per_page=${limit}&page=${page}`;
return await this.get(url);
}

private async get(url: string): Promise<any> {
let headers = {};
if (this.hasApiToken()) {
headers = {
Authorization: `Bearer ${this.githubToken}`,
};
}
const response = await fetch(url, {
headers: headers,
});
const json: any = await response.json();
return json;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you consider using the octokit and actions toolkit provided by GitHub to make requests?
(You can find an example of such an implementation here https://github.com/actions/toolkit/tree/main/packages/github)

Copy link
Author

Choose a reason for hiding this comment

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

Will take a look, thanks for the reference!

@fwal fwal added the enhancement New feature or request label Sep 9, 2024
function makeSnapshotPackage(snapshot: Snapshot, system: System): Package {
if (snapshot.branch === "main") {
return makePackage(
"6.0",
Copy link
Author

Choose a reason for hiding this comment

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

@fwal this one puzzles me on how to address: right now main snapshots has already moved to 6.1, and action still have hard-coded 6.0 version, and obviously this will be the issue for any next release as long as this hard-coded. And action is currently relies on this version for caching (which for main-snapshots in general questionable). From my perspective, there are two solutions: 1 - keep it manually updated, as I don't see a significant troubles for that cases; 2 - disable caching for main-branch snapshots and therefore the need to specify version. What do you think on these options, or maybe there are other ways to solve this?

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

Successfully merging this pull request may close these issues.

4 participants