-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement internal APIs for list/get/download artifacts #1591
Conversation
maxAttempts, | ||
baseRetryIntervalMilliseconds, | ||
retryMultiplier | ||
getUserAgentString(), |
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.
I chose to change this, since the user agent header isn't always going to be for the upload or download artifact actions.
Having the pkg version will be more useful IMO
@@ -44,21 +44,9 @@ export async function uploadArtifact( | |||
|
|||
// get the IDs needed for the artifact creation | |||
const backendIds = getBackendIdsFromToken() | |||
if (!backendIds.workflowRunBackendId || !backendIds.workflowJobRunBackendId) { |
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.
Removed this since we'll be getting the backend ids in multiple client methods. I'll leave it up to the caller instead to handle the rejected promise. The behavior of having it resolve and requiring a check on success
is a bit odd IMO
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.
Overall looks good! Just some small questions
Part of:
For list/get/download operations, we'll use the internal runtime JWT and validate the claims to determine artifact resource access & fetch internally. This is a minor improvement so that
actions:read
permissions (not default) are not required to get/list/download Artifacts that currently scoped to the current run. This is the same behavior to how lookups on Artifacts work today.In the future, it'd be great if we can solely rely on public APIs with new fine grained
GITHUB_TOKEN
permissions, but there are other efforts blocking it at this time.We'll still offer the ability to use the public API for convenience, each of the Artifact client methods has
options
which has an intersection type withFindOptions
:toolkit/packages/artifact/src/internal/shared/interfaces.ts
Lines 129 to 142 in a59f976
We'll expose those as optional inputs on download-artifact, so there's a first class way to download artifacts from other repositories, e.g.