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

Artifact up/download to/from Azure Blob Storage #2318

Merged
merged 6 commits into from Aug 29, 2023

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Aug 23, 2023

This adds Azure Blob Storage as an accepted destination for artifacts.

To start with, it uses DefaultAzureCredential. This is fairly flexible, but unless the container is public, uploaded URLs cannot be accessed by default.

In the third commit I add support for account keys (shared key credentials), which can also be configured using a connection string. The shared key is mandatory for generating SAS (shared access signatures) URLs, which include a token allowing temporary access to a blob.

@DrJosh9000 DrJosh9000 force-pushed the pdp-1504-artifacts-in-azure branch 9 times, most recently from 744fc46 to 85cd66f Compare August 28, 2023 01:47
@DrJosh9000 DrJosh9000 marked this pull request as ready for review August 28, 2023 01:47
@DrJosh9000 DrJosh9000 force-pushed the pdp-1504-artifacts-in-azure branch 3 times, most recently from 260df95 to d2fd7f9 Compare August 28, 2023 02:16
@DrJosh9000 DrJosh9000 changed the title WIP: Artifact upload to Azure Blob storage Artifact up/download to/from Azure Blob storage Aug 28, 2023
@DrJosh9000 DrJosh9000 requested a review from a team August 28, 2023 02:25
Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

Looks good in general, I have a suggestion which I think you should implement. I guess not all the proposed AzureBlobLocation struct is needed in all the places it would have to be plumbed to, but I think it's still useful to create this structure.

@@ -10,5 +12,5 @@ type Uploader interface {
URL(*api.Artifact) string

// The actual uploading of the file
Upload(*api.Artifact) error
Upload(context.Context, *api.Artifact) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay!

internal/artifact/azure_blob.go Outdated Show resolved Hide resolved
internal/artifact/azure_blob_downloader.go Show resolved Hide resolved
internal/artifact/azure_blob_downloader.go Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Loving this internal package party we are having!

internal/artifact/azure_blob.go Outdated Show resolved Hide resolved
@DrJosh9000 DrJosh9000 changed the title Artifact up/download to/from Azure Blob storage Artifact up/download to/from Azure Blob Storage Aug 28, 2023
Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

clicommand/global.go Show resolved Hide resolved
@DrJosh9000 DrJosh9000 merged commit 585e4b2 into main Aug 29, 2023
1 check passed
@DrJosh9000 DrJosh9000 deleted the pdp-1504-artifacts-in-azure branch August 29, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants