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 missing GlanceAPI webhook #363

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

fmount
Copy link
Contributor

@fmount fmount commented Nov 4, 2023

In general it is not allowed to create a glanceAPI instance without defining the top-level Glance CR.
However, if for any, unknown reason a glanceAPI CR is created, it will always fail because of the required .spec.ContainerImage field that appears to be empty or simply not defined.
This patch introduces the glanceAPI missing webhook that covers at the glanceAPI level the ContainerImage propagation/definition.

@@ -5,7 +5,6 @@ metadata:
spec:
serviceUser: glance
serviceAccount: glance
containerImage: quay.io/podified-antelope-centos9/openstack-glance-api:current-podified
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dprince we can use this patch as a follow up of PR #358 and remove this line here using and not fail because of the glanceAPI webhook.

@fmount
Copy link
Contributor Author

fmount commented Nov 6, 2023

/test glance-operator-build-deploy-tempest

// log is for logging in this package.
var glanceapilog = logf.Log.WithName("glanceapi-resource")

// SetupGlanceAPIDefaults - initialize Glance spec defaults for use with either internal or external webhooks
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/initialize Glance/initialize GlanceAPI/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, updating it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@fmount
Copy link
Contributor Author

fmount commented Nov 6, 2023

/test glance-operator-build-deploy-tempest

Comment on lines +36 to +40
// SetupGlanceAPIDefaults - initialize GlanceAPI spec defaults for use with either internal or external webhooks
func SetupGlanceAPIDefaults(defaults GlanceAPIDefaults) {
glanceAPIDefaults = defaults
glancelog.Info("Glance defaults initialized", "defaults", defaults)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also need something that pulls-in the ENV vars and sets them in the GlanceAPIDefaults, like we do here [1]. Maybe I just am missing where you're already doing this, however.

[1]

func SetupDefaults() {
// Acquire environmental defaults and initialize Glance defaults with them
glanceDefaults := GlanceDefaults{
ContainerImageURL: util.GetEnvVar("RELATED_IMAGE_GLANCE_API_IMAGE_URL_DEFAULT", GlanceAPIContainerImage),
}
SetupGlanceDefaults(glanceDefaults)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I need that function too in common.go. Updating the patch in a moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

In general it is not allowed to create a glanceAPI instance without
defining the top-level Glance CR.
However, if for any, unknown reason a glanceAPI CR is created, it
will always fail because of the required .spec.ContainerImage field
that appears to be empty or simply not defined.
This patch introduces the glanceAPI missing webhook that covers at
the glanceAPI level the ContainerImage propagationa/definition.

Signed-off-by: Francesco Pantano <[email protected]>
@fmount
Copy link
Contributor Author

fmount commented Nov 6, 2023

/test glance-operator-build-deploy-tempest

@fmount fmount requested a review from abays November 6, 2023 17:43
@openshift-ci openshift-ci bot added the lgtm label Nov 6, 2023
Copy link
Contributor

openshift-ci bot commented Nov 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprince, fmount

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

@openshift-merge-bot openshift-merge-bot bot merged commit f304ac5 into openstack-k8s-operators:main Nov 6, 2023
1 check 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.

3 participants