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

add tags #236

Merged
merged 1 commit into from
Oct 24, 2023
Merged

add tags #236

merged 1 commit into from
Oct 24, 2023

Conversation

rgl
Copy link
Contributor

@rgl rgl commented Oct 8, 2023

This adds support for tags.

To set the tags, set the tags property value to a semicolon separated string. For example:

source "proxmox-iso" "debian-amd64" {
  template_name            = "template-debian-${var.version}"
  template_description     = "See https://github.com/rgl/debian-vagrant"
  tags                     = "debian-${var.version};template" # TODO declare this as an array instead?

I'm still unsure whether to support an array instead, i.e., "debian-${var.version};template" vs ["debian-${var.version", "template"]. Please advice.

Closes #193

@rgl rgl requested a review from a team as a code owner October 8, 2023 21:26
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 @rgl,

Thanks for the PR, looking at the code, the current implementation directly passes through the data to the Proxmox API, so it may make sense to keep it this way for transparency, although I would think an array of strings here to be more idiomatic to Packer, and maybe more resilient to future changes (although I would be surprised if this ever changes).

I'll let you decide if we keep the current implementation or if we move to a string array based implementation with a stirings.Join before pushing it to the API, my preference goes for the latter, but the former may make more sense for people who have qemu experience.

Alternatively may I ask what it looks like on PVE? I don't have an instance to access, but their way to specify the tags should probably be what we adopt for this feature.

@@ -93,6 +93,9 @@ type Config struct {
// If not given, the next free ID on the cluster will be used.
VMID int `mapstructure:"vm_id"`

// The tags to set.
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, if we keep the current format with values separated by ;, that we disclose it here so it's clearly described in the docs.

@rgl
Copy link
Contributor Author

rgl commented Oct 10, 2023

I've ended up using the semicolon separated list.

At the Proxmox VE Management Console, this is how it looks (notice the tags above the Debian logo):

image

At the VM definition file, this is how it looks (notice the line that starts with tags: near the bottom):

root@pve:~# cat /etc/pve/qemu-server/105.conf 
#Packer ephemeral build VM
agent: 1
bios: ovmf
boot: order=scsi0;ide2;net0
cores: 2
cpu: host
efidisk0: local-lvm:vm-105-disk-0,efitype=4m,pre-enrolled-keys=0,size=4M
ide2: local:iso/debian-12.2.0-amd64-netinst.iso,media=cdrom,size=628M
kvm: 1
machine: q35
memory: 2048
meta: creation-qemu=8.0.2,ctime=1696964183
name: packer-65259e1b-00ca-3a70-468a-6c52197d9ba9
net0: virtio=AE:BF:35:97:95:D4,bridge=vmbr0
numa: 0
onboot: 0
ostype: l26
scsi0: local-lvm:vm-105-disk-1,discard=on,iothread=1,size=8G,ssd=1
scsihw: virtio-scsi-single
smbios1: uuid=d10b8b20-da59-4d94-8ac0-0e5b16a32773
sockets: 1
tags: debian-12;template
vga: memory=16,type=qxl
vmgenid: 52bc616a-ae72-43f8-afec-353f060881b0

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 @rgl,

Thanks for pushing this! The code looks good to me, I'll merge this now

@lbajolet-hashicorp lbajolet-hashicorp merged commit 956f37b into hashicorp:main Oct 24, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxmox plugin doesn't support tags
2 participants