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

image-generator: skip image creation for unsupported providers #362

Merged
merged 1 commit into from Nov 22, 2023

Conversation

snir911
Copy link
Contributor

@snir911 snir911 commented Nov 22, 2023

this allows installtion completion without being blocked waiting
for image creation when image creation isn't implemented for
cloud-provider

@snir911 snir911 self-assigned this Nov 22, 2023
@snir911 snir911 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023
Copy link
Member

@gkurz gkurz left a comment

Choose a reason for hiding this comment

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

Thanks @snir911 ! The change seems to be good but I still have some comments :

  1. please add some context in the commit log : motivation, link to jira...
  2. this moves setupCloudProvider() into newImageGenerator() : not sure to understand why and this makes the change harder to review. Could the refactoring happen in a preliminary commit ?

controllers/image_generator.go Outdated Show resolved Hide resolved
controllers/image_generator.go Outdated Show resolved Hide resolved
@snir911 snir911 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023
this allows installtion completion without being blocked waiting
for image creation when image creation isn't implemented for
cloud-provider

Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
@gkurz
Copy link
Member

gkurz commented Nov 22, 2023

Now waiting for @tbuskey to pre-merge test this PR for regressions with supported providers.

@tbuskey
Copy link
Contributor

tbuskey commented Nov 22, 2023

@gkurz @snir911 Could this be tested by setting CLOUD_PROVIDER: "xyzzy" in peer-pods-cm even it its running on azure or AWS? And I could see that the job doesn't get created when I install kataconfig?

@snir911
Copy link
Contributor Author

snir911 commented Nov 22, 2023

@tbuskey , no, it fetches the cloud-provider name from the the infrastructure object of the cluster

@gkurz
Copy link
Member

gkurz commented Nov 22, 2023

@tbuskey
Copy link
Contributor

tbuskey commented Nov 22, 2023

Azure works
AWS works
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2023
@gkurz gkurz merged commit 5a53f5d into openshift:devel Nov 22, 2023
@gkurz gkurz mentioned this pull request Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants