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

feat: allow vmtemplate selection based on tags #343

Merged
merged 17 commits into from
Feb 5, 2025
Merged

feat: allow vmtemplate selection based on tags #343

merged 17 commits into from
Feb 5, 2025

Conversation

pborn-ionos
Copy link
Contributor

@pborn-ionos pborn-ionos commented Dec 2, 2024

  • (sourceNode + templateID) and templateSelector are supposed to be mutually exclusive
  • automatically detects both sourceNode + templateID
  • errors out if anything but one (1) VM template with desired flags was found

Issue #, if available:
#338

Description of changes:
Allows selecting the vm template via

spec:
  template:
    spec:
      templateSelector:
        matchTags:
        - capi
        - capmox
        - template
        - v1.28.3

on ProxmoxMachineTemplate objects.

Testing performed:

  • successfully provisioned machines using the existing sourceNode+templateID and the new templateSelector options
  • successfully errored out, when multiple, or zero, templates with matching tags were found

Copy link

github-actions bot commented Dec 2, 2024

🚀 e2e tests run

We add labels to the PRs to control the e2e test runs by running specific tests and skipping some test contexts,
please follow this guide:

Label Behaviour
none Run Generic tests
e2e/none skip all e2e tests (documentation etc) - overrides all e2e/* labels Do not run any tests (overrides all e2e/ flags)
e2e/flatcar run Flatcar e2e tests Add Flatcar tests to the run

ℹ️ Ask a Member to add the requested labels if you don't have enough permissions.

@pborn-ionos pborn-ionos marked this pull request as ready for review December 3, 2024 14:43
@wikkyk
Copy link
Collaborator

wikkyk commented Feb 5, 2025

Hm indeed that's not how I'd expect selection by tag to work, either. I shouldn't need to pass the exact tags, just enough tags to get a result.

@mcbenjemaa
Copy link
Member

Hm indeed that's not how I'd expect selection by tag to work, either. I shouldn't need to pass the exact tags, just enough tags to get a result.

what about multiple VMs has the same matchTags
what would you suggest in this case?

@wikkyk wikkyk dismissed their stale review February 5, 2025 15:23

discussion ongoing

@wikkyk
Copy link
Collaborator

wikkyk commented Feb 5, 2025

You need to pass enough tags to get a unique result. I agree with you though that you shouldn't need to pass all the tags a template has.

Copy link

github-actions bot commented Feb 5, 2025

External PR

Test runs on external PRs require manual approval.

@wikkyk
Copy link
Collaborator

wikkyk commented Feb 5, 2025

We had a chat about this and decided to keep this behaviour and document it.

Copy link

github-actions bot commented Feb 5, 2025

External PR

Test runs on external PRs require manual approval.

mcbenjemaa
mcbenjemaa previously approved these changes Feb 5, 2025
Copy link
Member

@mcbenjemaa mcbenjemaa left a comment

Choose a reason for hiding this comment

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

Fix the lint and you are good to go.

Copy link

github-actions bot commented Feb 5, 2025

External PR

Test runs on external PRs require manual approval.

@wikkyk wikkyk enabled auto-merge (squash) February 5, 2025 15:58
Copy link

github-actions bot commented Feb 5, 2025

External PR

Test runs on external PRs require manual approval.

Note: This PR changes the following non-go, non-docs files:
config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxclusters.yaml
config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxclustertemplates.yaml
config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachines.yaml
config/crd/bases/infrastructure.cluster.x-k8s.io_proxmoxmachinetemplates.yaml

@mcbenjemaa mcbenjemaa disabled auto-merge February 5, 2025 16:01
@wikkyk wikkyk enabled auto-merge (squash) February 5, 2025 16:01
@wikkyk wikkyk merged commit 604ae96 into main Feb 5, 2025
11 checks passed
@wikkyk wikkyk deleted the 338 branch February 5, 2025 16:25
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.

4 participants