Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for uploading an ISO for building #208

Merged
merged 6 commits into from
Jan 19, 2023

Conversation

jasites
Copy link
Contributor

@jasites jasites commented Jan 17, 2023

Description

Adds iso_url option to use the create-iso API endpoint to upload an ISO, updates the Config struct to use the newly created ISO's ID for the instance, and deletes the ISO upon completion/cancellation

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally?

@optik-aper optik-aper self-assigned this Jan 17, 2023
@optik-aper optik-aper added the enhancement New feature or request label Jan 17, 2023
@optik-aper
Copy link
Member

optik-aper commented Jan 18, 2023

This needs to handle invalid configs upfront. For instance, using this config:

packer {
  required_plugins {
  }
}

build {

  sources = [
    "source.vultr.iso"
  ]
}

variable "vultr_api_key" {
  type    = string
  default = "${env("VULTR_API_KEY")}"
}

source "vultr" "iso" {
  api_key              = "${var.vultr_api_key}"
  os_id                = 387
  iso_url              = "https://dl-cdn.alpinelinux.org/alpine/v3.17/releases/x86_64/alpine-virt-3.17.1-x86_64.iso"
  region_id            = "ewr"
  plan_id              = "vhf-2c-4gb"
  snapshot_description = "iso-test"
  state_timeout        = "20m"
  ssh_username         = "root"
}

os_id and iso_url shouldn't be allowed in the same config. The plugin proceeds but, eventually, the API will reject:

michael  ~/code-review/packer-plugin-vultr/packer-plugin-vultr-cust-128-iso (:|✔)
 Φ packer build iso-test.pkr.hcl                                                                                                 115:45
vultr.ubuntu: output will be in this color.

==> vultr.ubuntu: Running Vultr builder...
==> vultr.ubuntu: Creating temporary SSH key...
==> vultr.ubuntu: Creating ISO in Vultr account...
==> vultr.ubuntu: Waiting 1200s for ISO alpine-virt-3.17.1-x86_64.iso (8f894776-7536-47bc-a824-3a9a364dcff2) to complete uploading...
==> vultr.ubuntu: Creating Vultr instance...
==> vultr.ubuntu: Error creating server: {"message":"please provide one app_id, image_id, iso_id, os_id, or snapshot_id","status":400}
==> vultr.ubuntu: Destroying ISO 8f894776-7536-47bc-a824-3a9a364dcff2
==> vultr.ubuntu: Deleting temporary SSH key...
Build 'vultr.ubuntu' errored after 15 seconds 606 milliseconds: Error creating server: {"message":"please provide one app_id, image_id, iso_id, os_id, or snapshot_id","status":400}

==> Wait completed after 15 seconds 606 milliseconds

==> Some builds didn't complete successfully and had errors:
--> vultr.ubuntu: Error creating server: {"message":"please provide one app_id, image_id, iso_id, os_id, or snapshot_id","status":400}

==> Builds finished but no artifacts were created.

Handle these exclusive cases before proceeding in the config prepare function.

@jasites jasites requested a review from optik-aper January 18, 2023 21:41
@optik-aper
Copy link
Member

What about doing this instead of the complicated && || chain?

	imageConfig := []bool{(c.ISOID != ""), (c.ISOURL != ""), (c.AppID != 0), (c.SnapshotID != ""), (c.OSID != 0)}
	imageDefined := false
	for _, isDefined := range imageConfig {
		if isDefined {
			if imageDefined {
				errs = packer.MultiErrorAppend(errs, errors.New("you can only set one of the following: `os_id`, `app_id`, `snapshot_id`, `iso_id`, `iso_url`"))
				break
			}
			imageDefined = true
		}
	}

This one will also handled os_id which should be exclusive.

@jasites
Copy link
Contributor Author

jasites commented Jan 19, 2023

Thanks for the feedback! Your suggestion definitely seems like it should do the trick in a simpler and more idiomatic manner... my inexperience with Go is showing 😉

@optik-aper
Copy link
Member

For sure! It took some figuring, and I'm still a little leery of people defining app_id or os_id as 0 in the configs since that will pass this check. We could update the mapstructure stuff to return the typed nil in that case but it seems like a minor enough case that I didn't want to rejigger the config values downstream.

Copy link
Member

@optik-aper optik-aper 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 and works locally.

@optik-aper optik-aper merged commit 30e6b6a into vultr:master Jan 19, 2023
@jasites jasites deleted the cust-128/iso branch January 19, 2023 17:21
@optik-aper optik-aper mentioned this pull request Apr 6, 2023
3 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants