-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 hcp_packer_registry block #11168
Conversation
b123a53
to
2ed61f8
Compare
f004e68
to
9943301
Compare
574e3d0
to
7e8756e
Compare
8dcb04e
to
31f9410
Compare
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.
This looks really good. I left a couple of questions.
4d55796
to
44a0393
Compare
c5090d7
to
5fc3802
Compare
5fc3802
to
81dad99
Compare
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.
This looks great! I left a few nits and one question. But this is otherwise good to go. Nicely done. Feel free to merge when ready.
"source.virtualbox-iso.ubuntu-1204", | ||
] | ||
|
||
source "source.amazon-ebs.ubuntu-1604" { |
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.
Nice touch. Great to see this being captured in the test 🐛 👮
} | ||
|
||
func (b *HCPPackerRegistryBlock) WriteBucketConfig(bucket *packerregistry.Bucket) { | ||
if b == nil { |
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.
💯
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.
Now that I learned this was possible 😆
website/content/docs/templates/hcl_templates/blocks/build/hcp_packer_registry.mdx
Outdated
Show resolved
Hide resolved
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.
Heya, nice going ! I just had a few comments/concerns there and there, but I like the direction.
name = "bucket-slug" | ||
|
||
hcp_packer_registry { | ||
description = <<EOT | ||
Some description | ||
EOT | ||
labels = { | ||
"foo" = "bar" | ||
} | ||
} |
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 think that the bucket setting should be specific and not be taken from the build block, this could get quite confusing ! Like what if I want to name my build with something but push to another bucket ?
name = "bucket-slug" | |
hcp_packer_registry { | |
description = <<EOT | |
Some description | |
EOT | |
labels = { | |
"foo" = "bar" | |
} | |
} | |
name = "build-name" | |
hcp_packer_registry { | |
push_to = "alpine" | |
description = <<EOT | |
Some description | |
EOT | |
labels = { | |
"foo" = "bar" | |
} | |
} |
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.
Another question on the same topic, why not taking the description from the build and put that in the bucket too ?
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 think that the bucket setting should be specific and not be taken from the build block, this could get quite confusing ! Like what if I want to name my build with something but push to another bucket ?
This is fair. From the minimal viable configuration option stand point " A user should be able to push to PAR simply by adding a few environment variables" so this keeps to this promise as long as a build name is part of the configuration template. Or the user provides a HCP_PACKER_BUILD_NAME
env variable along side the HCP creds.
That said I think that supporting a different build name in the future (it would be great to get beta feedback here) can be a simple thing to do. But for now it feels a little out of scope. Of course happy to be told differently.
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.
Nice! I will make the same logic for both slug and description. If not present in hcp_packer_registry, then use the ones from build.
Also, the description is not documented in the deployed docs. Is it missing or is it something new?
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.
(talking to Wilken) I'd think the opposite, I think it would be quite easier to make of that a variable, and then pass it to the build and hcp blocks, to not repeat myself.
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.
pinging @SwampDragons as well for thoughts on last question.
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.
Adding a note here that I initially expected component type to be just the component that built that thing; so the docker
builder, the amazon-ebs
builder or the google-import
pp; for example.
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.
To me, this is marketing, not a technical spec 😛
This is a technical spec. Otherwise it's not marketing, it's a lie.
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.
When writing the service, I imagined "component-type" as literally the builder or post-processor that created the builder, without either name
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.
Ah, yes, what Adrien said.
…packer_registry.mdx Co-authored-by: Wilken Rivera <wilken@hashicorp.com>
* parse hcp_packer_registry block * Add config to hcp_packer_registry block * get org and proj id from client secrets * move org and proj id logic to client * clean up client config * rename InPARMode to HasHCPCredentials * add HCP_PACKER_REGISTRY back to enable PAR when hcp block not present * validate credentials when creating client * fix - allow hcl2 config par enable without block * fix build registration * update error msg * fix IsPAREnabled logic * fix par enabled and bucket validation * update hcp-sdk-go * add hcp_packer_registry tests * add some doc in the code * add packer.io docs * Update website/content/docs/templates/hcl_templates/blocks/build/hcp_packer_registry.mdx Co-authored-by: Wilken Rivera <wilken@hashicorp.com> * remove copy and paste * add slug option to hcp_packer_registry * add slug docs Co-authored-by: Wilken Rivera <wilken@hashicorp.com>
* parse hcp_packer_registry block * Add config to hcp_packer_registry block * get org and proj id from client secrets * move org and proj id logic to client * clean up client config * rename InPARMode to HasHCPCredentials * add HCP_PACKER_REGISTRY back to enable PAR when hcp block not present * validate credentials when creating client * fix - allow hcl2 config par enable without block * fix build registration * update error msg * fix IsPAREnabled logic * fix par enabled and bucket validation * update hcp-sdk-go * add hcp_packer_registry tests * add some doc in the code * add packer.io docs * Update website/content/docs/templates/hcl_templates/blocks/build/hcp_packer_registry.mdx Co-authored-by: Wilken Rivera <wilken@hashicorp.com> * remove copy and paste * add slug option to hcp_packer_registry * add slug docs Co-authored-by: Wilken Rivera <wilken@hashicorp.com>
* parse hcp_packer_registry block * Add config to hcp_packer_registry block * get org and proj id from client secrets * move org and proj id logic to client * clean up client config * rename InPARMode to HasHCPCredentials * add HCP_PACKER_REGISTRY back to enable PAR when hcp block not present * validate credentials when creating client * fix - allow hcl2 config par enable without block * fix build registration * update error msg * fix IsPAREnabled logic * fix par enabled and bucket validation * update hcp-sdk-go * add hcp_packer_registry tests * add some doc in the code * add packer.io docs * Update website/content/docs/templates/hcl_templates/blocks/build/hcp_packer_registry.mdx Co-authored-by: Wilken Rivera <wilken@hashicorp.com> * remove copy and paste * add slug option to hcp_packer_registry * add slug docs Co-authored-by: Wilken Rivera <wilken@hashicorp.com>
* Add working registry pkg * Add custom error for handling the loading of PAR environment variables * Working Publish to Build, with proper error handling for bucket names * Update hcp-sdk-go to use branch instead of mod replace directive * Update Packer build status configuration * Add support for HCP_PACKER_BUILD_FINGERPRINT env * Add support for publishing one or more PARtifacts from a single build * add git shas to this branch * Add ability to set provider name if available * Add working RegistryBuilder type * Add RegistryPostProcessor as wrapper post-processor * When in PAR mode a empty RegistryPostProcessor is added to the end of the post-processor list to publish all final image data. * Add support for updating a build from PAR that is not in a DONE state * Fix a small issue with creation the initial builds for an empty iteration. * Add PAR URL to post-processor display * Implement hcp_packer_registry block (#11168) * Update vendored Amazon plugin to v1.0.1-dev * Fix panic when running a Packer registry build in a clean directory * Remove the publishing of post-processor metadata from the registry post-processor. * Remove metadata add from registry_builder * Update registry builder to skip a build that was found to be DONE Co-authored-by: Megan Marsh <megan@hashicorp.com> Co-authored-by: Sylvia Moss <moss@hashicorp.com>
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Testing template