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 connection_string secret to default_user secret #1721

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

hxyannay
Copy link
Contributor

@hxyannay hxyannay commented Sep 2, 2024

This closes #1722

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

Summary Of Changes

Added a new key connection_string to the default_user secret with a formatted connection_string.

Copy link
Collaborator

@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.

Thank you for this contribution. I like this idea. I left some cosmetic suggestions.

internal/resource/default_user_secret_test.go Outdated Show resolved Hide resolved
internal/resource/default_user_secret_test.go Outdated Show resolved Hide resolved
@Zerpet Zerpet self-assigned this Sep 5, 2024
@Zerpet Zerpet added this to the v2.11.0 milestone Sep 5, 2024
@hxyannay hxyannay force-pushed the main branch 2 times, most recently from 8303468 to 99f123a Compare September 6, 2024 08:00
Copy link
Collaborator

@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.

It is looking better 👍 I left two suggestions before approving this PR.

internal/resource/default_user_secret_test.go Outdated Show resolved Hide resolved
internal/resource/default_user_secret_test.go Outdated Show resolved Hide resolved
@hxyannay
Copy link
Contributor Author

hxyannay commented Sep 9, 2024

Committed your changes :)
Also it would be great if you could release a new version of the operator once you merge this pull request, so I could use those changes right away.

@hxyannay
Copy link
Contributor Author

Hey @Zerpet
Can you take another look?
Thanks!

@Zerpet
Copy link
Collaborator

Zerpet commented Sep 12, 2024

I approved the PR ✅ I'll merge after CI passes. We released 2.10.0 last week, and it is a bit soon to release another version. We tend to wait for 1 month between minor releases. Plus we have some dependabot PRs that need manual intervention.

If you want to use these changes right away, I'd recommend that you build own "snapshot" version from main after merging. There's a Make target to build, and it only requires 2 inputs, see:

cluster-operator/Makefile

Lines 207 to 210 in 86e948e

docker-build: ## Build the docker image with tag `latest`
@$(call check_defined, OPERATOR_IMAGE, path to the Operator image within the registry e.g. rabbitmq/cluster-operator)
@$(call check_defined, DOCKER_REGISTRY_SERVER, URL of docker registry containing the Operator image e.g. registry.my-company.com)
docker buildx build --build-arg=GIT_COMMIT=$(GIT_COMMIT) -t $(DOCKER_REGISTRY_SERVER)/$(OPERATOR_IMAGE):latest .

You can build the operator locally, for example, with:

make docker-build DOCKER_REGISTRY_SERVER=docker.io OPERATOR_IMAGE=hxyannay/rabbitmq-cluster-operator

You will have to push the image then with docker push and update the latest release manifests to use this image.

@hxyannay
Copy link
Contributor Author

Great, I will build an image myself for the time being.
Thank you.

@Zerpet Zerpet merged commit b17bec0 into rabbitmq:main Sep 13, 2024
16 checks passed
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.

Add connection_string to the default_user secret
2 participants