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

Use Object Storage URL Presigning for image create #87

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kuzpactor
Copy link
Contributor

This PR extends presignUrl() function usage to include file-based sources to be uploaded on Object Storage.

Closes #86

@kuzpactor kuzpactor requested a review from a team as a code owner August 20, 2023 19:01
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @kuzpactor,

Looks good to me, I just left a comment regarding the check for the empty Bucket that can be moved to Configure to get caught earlier in the build process, but this is not strictly linked to your PR so this can be addressed separately.

The change proposed may however slightly conflict with what you proposed in PR #84, won't it?

I would suggest merging one and then the other, and rebasing the latter on top of the current main. The order is not important in my opinion, so feel free to pick which one to merge first.

Thanks for the PR!

@@ -139,10 +139,14 @@ func (p *PostProcessor) PostProcess(ctx context.Context, ui packersdk.Ui, artifa

// As `bucket` option validate input here
if p.config.Bucket == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest moving this to Configure maybe so it is handled before the build gets executed, since this is a configuration issue, it should be caught as early as possible to not waste user time.

Note: this is not directly related to this code, so this can be done in another patchset later.

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.

Presigned Storage URLs not used for image generation
3 participants