-
Notifications
You must be signed in to change notification settings - Fork 65
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
Cluster setup #42
base: main
Are you sure you want to change the base?
Cluster setup #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically looks good - thanks! There are a few edge cases to handle:
_up
might be protectd byrequire_valid_user: true
.adminPassword
might not match what's deployed. This is a hangover from allowing users to set the admin hash explicitly to ensure that all pods use an identical value, but means they need to ensure the admin password is also passed to the chart.
We can verify the behaviour by running the Kind
tests and adding a new scenario to https://github.com/apache/couchdb-helm/tree/master/couchdb/ci
E2E tests do pass for me, once I apply this small patch. Should I add this to the PR? --- a/test/kind-config.yaml
+++ b/test/kind-config.yaml
@@ -1,5 +1,5 @@
kind: Cluster
-apiVersion: kind.sigs.k8s.io/v1alpha3
+apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
- role: worker |
Is there anything we can do about this, other than documenting the behavior? |
Yes please. Bear in mind the tests will use the default values, so won't execute with |
I seem to have spoken prematurely about the passage of E2E tests. I see now there is an error. I get the same error when running the tests against
|
I wonder if this has rotted against the latest
|
7c81a4c
to
14394f2
Compare
This does mean that we now send credentials for the /_up endpoint even when require_valid_user is not set, but this should not hurt anything.
14394f2
to
9a22f3f
Compare
I ran into this same issue, is this fix still being considered? |
What this PR does / why we need it:
This PR adds the
clusterSetup
variable, which defaults to false. When enabled, a Helmpost-install
hook is initialized which waits for the_up
endpoint to respond with a 200, then finalizes cluster setup by sending the requisite POST to/_cluster_setup
.It's quite possible there are better ways to do some of the things I've done here. I'm open to any and all feedback.
Which issue this PR fixes
Fixes #41
Special notes for your reviewer:
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.