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

Expose internalArtifactTwirpClient options (especially maxAttempts) to uploadArtifact and friends #1864

Open
jsoref opened this issue Oct 30, 2024 · 0 comments
Labels
enhancement New feature or request

Comments

@jsoref
Copy link
Contributor

jsoref commented Oct 30, 2024

Describe the enhancement

The current exported uploadArtifact api:

export async function uploadArtifact(
name: string,
files: string[],
rootDirectory: string,
options?: UploadArtifactOptions | undefined

Takes a parameter

export interface UploadArtifactOptions {

That only exposes the two fields visible once an artifact is created:

And it passes no arguments to internalArtifactTwirpClient

const artifactClient = internalArtifactTwirpClient()

But, internalArtifactTwirpClient takes maxAttempts, retryIntervalMs, and retryMultiplier:

export function internalArtifactTwirpClient(options?: {
maxAttempts?: number
retryIntervalMs?: number
retryMultiplier?: number
}): ArtifactServiceClientJSON {

Code Snippet

export interface UploadArtifactSettings {
   maxAttempts?: number,
   retryIntervalMs?: number,
   retryMultiplier?: number, 
}

export interface UploadArtifactOptions { 
retentionDays?: number, 
compressionLevel?: number,
uploadSettings?: UploadArtifactSettings,
}
uploadArtifact("my-name", ["my-file"], ".", { uploadSettings: { maxAttempts: 2 } });

I think there's some value in distinguishing between values which are visible after an artifact is uploaded with the values for how the artifact is uploaded, hence sticking them in a sub object. I lean here towards extending the current options argument instead of adding a second options argument to the uploadArtifact call itself. But any approach would be fine.

Additional information
There are people who trip on outages from this api, as seen in actions/upload-artifact#569.

I'd like to be able to configure my actions/workflows to give up sooner (or try longer, but, in my case definitely give up sooner) when uploading an artifact fails.

In my case, two failed artifact uploads added 6 minutes to a 4 minute run. If I had CI for a private repository doing that, I'd expect those 6 minutes to be billed and that could add up over time.

While I'm mostly interested in exposing maxAttempts to uploadArtifact (and eventually getting it added to actions/upload-artifact), I think exposing retryIntervalMs and retryMultiplier isn't unreasonable. Similarly, I'd want to be able to use these options for downloadArtifact and listArtifacts as well...

uploadArtifact(
name: string,
files: string[],
rootDirectory: string,
options?: UploadArtifactOptions
): Promise<UploadArtifactResponse>

listArtifacts(
options?: ListArtifactsOptions & FindOptions
): Promise<ListArtifactsResponse>

export async function downloadArtifactInternal(
artifactId: number,
options?: DownloadArtifactOptions
): Promise<DownloadArtifactResponse> {
const downloadPath = await resolveOrCreateDirectory(options?.path)
const artifactClient = internalArtifactTwirpClient()

export async function listArtifactsInternal(
latest = false
): Promise<ListArtifactsResponse> {
const artifactClient = internalArtifactTwirpClient()


If exposing them directly through the API is too much, I'd be happy to have them exposed via environment variables as that would save me the effort of getting the middle consumers (actions/upload-artifact,...) to update and expose the knobs.

@jsoref jsoref added the enhancement New feature or request label Oct 30, 2024
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

No branches or pull requests

1 participant