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

Simplify secrets to avoid dependencies on a file and keep only the se… #104

Closed
wants to merge 3 commits into from
Closed

Conversation

rileyai-dev
Copy link
Contributor

@rileyai-dev rileyai-dev commented Dec 19, 2022

What this PR does / why we need it:

It simplifies the secret. Instead of relying on password.ini for the adminHash, it simply stores the value. It helps users better manage their own secret files when they set createAdminSecret to false.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.

  • Chart Version bumped
  • e2e tests pass
  • Variables are documented in the README.md

@yekibud
Copy link
Contributor

yekibud commented Dec 22, 2022

Hi @rileyai-dev . I've been getting the following on my PRs trying to run the e2e tests. It appears these tests are passing without any errors for you? Is there anything special about the way you runt the tests? I just run make test from the project root.
image

@rileyai-dev
Copy link
Contributor Author

rileyai-dev commented Dec 23, 2022

Hi @yekibud,

Thanks for the reply! Actually I haven't tried the test... I have created my own chart package and I am currently using it in my environment. I see that the test you run is mentioning 3.6.3 while I have bumped the version of the chart to 3.7.0 (I did so to reflect the change for users who might already have an existing secret).

Also I haven't built the package in this repo since I couldn't find instructions... so the index doesn't include this new chart and the package doesn't exist. Would appreciate if someone could package it.

@yekibud
Copy link
Contributor

yekibud commented Dec 23, 2022

Oh, okay, I thought you had run the test because the "e2e tests pass" checkbox was checked in the PR. If you get a chance to run make test I would be curious to see if it passes for you or not.

I think building the package should just be creating a new tarball and updating the index with helm repo index docs. However, this doesn't seem to be updating the helm repo itself, when PRs are merged, which I noted in my current PR. So not sure if there's something wrong with the CI or a step I'm missing, as well.

@rileyai-dev
Copy link
Contributor Author

@willholley Would highly appreciate if you could have a look at this. I added a new autoSetup feature to automatically finalize the cluster (set to false by default). It's very useful for fluxCD or equivalent tools!

@willholley
Copy link
Member

@rileyai-dev the autoSetup feature seems like it should be a distinct PR, else it's tricky to untangle the changes related to each enhancement for review.

@rileyai-dev rileyai-dev closed this Jan 8, 2023
@rileyai-dev
Copy link
Contributor Author

@willholley I have created a new PR-106 for the admin hash only. I will submit a new one for autoSetup asap.

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.

3 participants