Skip to content

Conversation

@coro
Copy link
Contributor

@coro coro commented Sep 28, 2021

Summary Of Changes

Instead of the default image for RabbitmqClusters being hard-coded into
the CRD, it is now available as a command-line flag to the operator's
manager binary. Changing the operator's Deployment manifest to call
/manager --default-rabbitmq-image image:tag will now cause all
RabbitmqClusters deployed by the operator to use that default image if
one is not specified in spec.image.

This closes #856

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Additional Context

This change will make it easier to deploy the operator using helm - the CRD itself will no longer change when bumping the default image, and so the raw CRD manifest can be placed in the crd/ directory, while still allowing the default image to be configurable on a per-operator-Deployment scope.

Local Testing

Please ensure you run the unit, integration and system tests before approving the PR.

To run the unit and integration tests:

$ make unit-tests integration-tests

You will need to target a k8s cluster and have the operator deployed for running the system tests.

For example, for a Kubernetes context named dev-bunny:

$ kubectx dev-bunny
$ make destroy deploy-dev
# wait for operator to be deployed
$ make system-tests

Instead of the default image for RabbitmqClusters being hard-coded into
the CRD, it is now available as a command-line flag to the operator's
`manager` binary. Changing the operator's Deployment manifest to call
`/manager --default-rabbitmq-image image:tag` will now cause all
RabbitmqClusters deployed by the operator to use that default image if
one is not specified in `spec.image`.

This closes #856
@coro coro marked this pull request as draft September 28, 2021 14:53
@coro coro marked this pull request as ready for review October 5, 2021 13:20
This is useful to test specific values for the operator. For example, to
test different values to `-default-rabbitmq-image`.

Signed-off-by: Aitor Perez Cedres <[email protected]>
Copy link
Member

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

Approving after doing acceptance and adding my own commit to the Makefile 🙈

@javsalgar
Copy link
Contributor

javsalgar commented Oct 7, 2021

Thank you so much for the PR! Just leaving a note in case it is useful.

When testing the Contour Operator (another operator that we have in our catalog) in our CI/CD, we found that in some Marketplaces like Azure Marketplace or AWS Marketplace the provided examples would not work out of the box because the imagePullSecrets were missing. In this sense, it is likely that this will happen with the RabbitMQ Cluster Operator as well (rigth now we are rendering the imagePullSecrets in the CRD but we want to get rid of templating the CRD), would it make sense to also allow setting a list of default imagePullSecrets to be set when deploying the RabbitMQCluster instance?

Otherwise, all users that would use the RabbitMQ Cluster Operator chart from Azure Marketplace or similar (which require imagePullSecrets) would not be able to run the default examples unless they specify the secrets.

Thoughts?

@coro coro merged commit b22be06 into main Oct 11, 2021
@coro coro deleted the default-image-flag branch October 11, 2021 15:03
coro added a commit that referenced this pull request Oct 11, 2021
This follows the same path as #858, and allows for easier helm integration
coro added a commit that referenced this pull request Oct 13, 2021
#867)

* Move default user updater image to CLI flag

This follows the same path as #858, and allows for easier helm integration

* Set the default image prior ot statefulset reconciliation

* Move CLI flags to env vars instead
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.

CRD needs templating which does not enable us to deploy the operator instance in the bitnami/rabbitmq-cluster-operator chart

5 participants