-
Notifications
You must be signed in to change notification settings - Fork 95
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
feat: import ovf template from url in supervisor builder #433
feat: import ovf template from url in supervisor builder #433
Conversation
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.
Thanks, Eric!
- Per the README.md, in case of feature contribution, we kindly ask you to open an issue to discuss it beforehand so that these can be linked.
- Could you use the Conventional Commits style and ensure it is signed off with your broadcom.com email attached to your GitHub account. The commit message should be short and you can leave this content in the linked PR.
- Also, please update
https://github.com/hashicorp/packer-plugin-vsphere/blob/main/docs/builders/vsphere-supervisor.mdx
to include the new items as this will then populate webdocs whenmake generate
is run.
700650e
to
0ab1c22
Compare
Thanks Ryan! Updated as below:
|
0ab1c22
to
c686984
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.
Minor items.
Thanks for the review, addressed the comments, please take a look again. |
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.
Thanks for addressing all the previous review comments! Have some additional thoughts to hopefully streamline the user experience of using this plugin with the import step.
1ad7ea6
to
79e429e
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.
LGTM with some final comments left. @ericvmw Thanks again for your patience in addressing all the reviews!
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.
Thanks @tenthirtyam @dilyar85 for the thorough reviews, truly appreciate it! |
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.
Deferring to @tenthirtyam and @dilyar85 on this given their already great review. Apologies for the delay on the merge.
4a88b65
to
ca80420
Compare
I manually resolved the conflicts and pushed. Once green I will merge. |
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.
Just left a few nitpicks mostly, but overall LGTM!
Pre-approving so it's not a blocker later
…er plugin This change is to add a configurable optional step on supervisor builder to allow users to import OVF template to Supervisor namespaces via packer builder vsphere supervisor plugin. Signed-off-by: Eric Cao <[email protected]>
1. make image_name optional, not required if image import is configured. 2. add separate configs `keep_import_request`, `import_target_image_name` for image import as sharing configs is not really supported.
ca80420
to
570d118
Compare
Hi @ericvmw thank you for addressing the few pieces of feedback. I will merge this today and release the plugin this week. If @tenthirtyam has no objections. |
Summary
A new feature is being added to Supervisors to expose a new custom resource
ContentLibraryItemImportRequest
that allows users to import OVF templates to a namespace with writable & import-allowed content library configured on a Supervisor, this enables users to use Packer to orchestrate a complete e2e workflow, from importing their source image from HTTP server, to customize and publish the image to Supervisor clusters.This change adds configurable optional steps on supervisor builder plugin to allow users to import OVF template to Supervisor namespaces via packer builder vsphere supervisor plugin.
Add import image step to import image from remote URL to the target content library in the specified namespace on the Supervisor by creating ContentLibraryItemImportRequest resource, wait until the import request completes. If keep_input_artifact is set to be true, the import request will be cleaned after it is done. Besides taking the image_name config from existing CreateSource step, this step also support the following optional configs:
import_source_url: the remote URL to import OVF template from, must starts with https.
import_source_ssl_certificate: the SSL certificate of the remote HTTPS server.
import_target_location_name: the import target location (content library that must be writable and allow import).
import_target_image_type: optional, the target image type, it defaults to OVF type and only OVF type is supported currently.
import_request_name: The name of the ContentLibraryItemImportRequest resource, otherwise it defaults to
packer-vsphere-supervisor-import-req-<random-suffix>
.watch_import_timeout_sec: The timeout in seconds to wait for the image to be imported, otherwise it defaults to
600
.clean_imported_image: optional, whether to clean the image imported in this step. If it is set to true, the imported image will be deleted after source VM is created and becomes ready. Defaults to false.
Add unit tests to cover both import validation and import image steps.
Note that in order to import OVF template from remote URL, the target content library must be configured with the default OVF security policy, and the OVF template must be signed with a certificated that is signed by a CA cert trusted by Content Library Service on vCenter, otherwise the import OVF template image will not be security compliant and cannot be deployed.
Testing
added unit tests pass:
Reference
Closes #434