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.

Sign up for GitHub

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

Jump to bottom

✨ Change API for OpenStackMachine.Spec.Image #1796

Merged
merged 1 commit into from
Jan 12, 2024
Merged

✨ Change API for OpenStackMachine.Spec.Image #1796

merged 1 commit into from
Jan 12, 2024

Conversation

EmilienM
Copy link
Contributor

What this PR does / why we need it:

Right now, the image for an OpenStackMachine can be defined either
its name via Spec.Image or via Spec.ImageUUID.

Now we will use a single parameter which would be of type ImageFilter.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1795

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

@netlify Netlify
Copy link

netlify bot commented Dec 18, 2023 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 4368c4f
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65a159d6359e2d00084ee56c
😎 Deploy Preview https://deploy-preview-1796--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 18, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 18, 2023
@EmilienM EmilienM force-pushed the issue_1795 branch 4 times, most recently from f880446 to 46b1937 Compare December 18, 2023 19:53
@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-test

@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

2 similar comments
@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

@EmilienM
Copy link
Contributor Author

EmilienM commented Dec 19, 2023 •
edited
Loading

the bastion instanceSpec gets wrong API:
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/1796/pull-cluster-api-provider-openstack-e2e-full-test/1737146179717697536/artifacts/clusters/clusterctl-upgrade-5awqmp/clusterctl-upgrade-7iukbs-cluster-template.yaml

---
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha6
kind: OpenStackCluster
metadata:
  name: clusterctl-upgrade-7iukbs
  namespace: clusterctl-upgrade
spec:
  allowAllInClusterTraffic: true
  apiServerLoadBalancer:
    enabled: true
  bastion:
    enabled: true
    instance:
      flavor: m1.tiny
      image:
        name: cirros-0.6.1-x86_64-disk

@EmilienM
Copy link
Contributor Author

I need to move test/e2e/data/kustomize/common-patches/cni/patch-cluster.yaml to versioned patches so it's not applied on v1alpha8.

@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

@EmilienM EmilienM force-pushed the issue_1795 branch 3 times, most recently from af51881 to 4f7f45a Compare December 20, 2023 19:31
@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

jichenjc
jichenjc reviewed Dec 21, 2023
View reviewed changes
Copy link
Contributor

@jichenjc jichenjc left a comment

Choose a reason for hiding this comment

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

the others lgtm

pkg/cloud/services/compute/instance.go Outdated Show resolved Hide resolved
dulek
dulek approved these changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2023
@EmilienM
Copy link
Contributor Author

@lentzi90 @mdbooth I have a preference to address this via another PR, which I'm working on now.

@EmilienM
Copy link
Contributor Author

/hold until I fix the bastion thing via #1822

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2024
@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

@EmilienM
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2024
@EmilienM EmilienM force-pushed the issue_1795 branch 3 times, most recently from 76346ab to 842679a Compare January 11, 2024 19:09
@EmilienM EmilienM mentioned this pull request Jan 11, 2024
Merged
@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test

@EmilienM
Copy link
Contributor Author

@lentzi90 @mdbooth ready for review again. Thanks!

lentzi90
lentzi90 reviewed Jan 12, 2024
View reviewed changes
test/e2e/shared/common.go Outdated Show resolved Hide resolved
lentzi90
lentzi90 reviewed Jan 12, 2024
View reviewed changes
Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/approve
The kustomize patches could be optimized a bit but I don't want to hold this back just because of that 🙂

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek, EmilienM, lentzi90, stephenfin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2024
@dulek
Copy link
Contributor

dulek commented Jan 12, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2024
@EmilienM
Convert Image to use ImageFilfter instead of string
4368c4f 
Right now, the image for an OpenStackMachine can be defined either
its name via Spec.Image or via Spec.ImageUUID.

Now we will use a single parameter which would be of type ImageFilter.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2024
@EmilienM
Copy link
Contributor Author

rebased

@MaysaMacedo
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2024
@k8s-ci-robot k8s-ci-robot merged commit 86f78d6 into kubernetes-sigs:main Jan 12, 2024
9 checks passed
@stephenfin stephenfin deleted the issue_1795 branch February 14, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@dulek dulek dulek approved these changes

@stephenfin stephenfin stephenfin approved these changes

@jichenjc jichenjc Awaiting requested review from jichenjc

@mdbooth mdbooth Awaiting requested review from mdbooth

Assignees

@dulek dulek

@stephenfin stephenfin

@MaysaMacedo MaysaMacedo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert Image & ImageUUID into a single param using an ImageFilter instead
8 participants
@EmilienM @MaysaMacedo @stephenfin @dulek @mdbooth @lentzi90 @k8s-ci-robot @jichenjc