Skip to content

feat: Add htpasswd generator using helm.#69

Merged
juanpicado merged 12 commits intoverdaccio:masterfrom
pshanoop:htpasswd
Jul 8, 2021
Merged

feat: Add htpasswd generator using helm.#69
juanpicado merged 12 commits intoverdaccio:masterfrom
pshanoop:htpasswd

Conversation

@pshanoop
Copy link
Copy Markdown
Collaborator

@pshanoop pshanoop commented Jul 1, 2021

  • Added support to have htpasswd credentails in helm values. So, a htpasswd file generated.

@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 1, 2021

Kind failed to create a node. Is the lint script broken or should I increase timeout for helm ?

@juanpicado
Copy link
Copy Markdown
Member

Fails on install, you might check the error

@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 1, 2021

Fails on install, you might check the error

I have already installed this on my cluster,
Did you check the log? https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:9:90 and
https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:8:30

Anyway, I will add small change to retrigger this.

@juanpicado juanpicado requested a review from a team July 1, 2021 19:00
htpasswd:
file: /verdaccio/storage/htpasswd
# Do not change this path if secrets htpasswd is used.
file: /verdaccio/auth/htpasswd
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

auth?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I thought, It makes sense to use auth for authentication and storage only for package storage.
Besides, it would conflict with storage PVC.

@juanpicado
Copy link
Copy Markdown
Member

Fails on install, you might check the error

I have already installed this on my cluster,
Did you check the log? https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:9:90 and
https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:8:30

Anyway, I will add small change to retrigger this.

I see this, I also triggered #70 and all is green.

Namespace 'verdaccio-xjtdpv878c' terminated.
Error: Error installing charts: Error processing charts
----------------------------------------------------------

@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 1, 2021

Fails on install, you might check the error

I have already installed this on my cluster,
Did you check the log? https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:9:90 and
https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:8:30
Anyway, I will add small change to retrigger this.

I see this, I also triggered #70 and all is green.

Namespace 'verdaccio-xjtdpv878c' terminated.
Error: Error installing charts: Error processing charts
----------------------------------------------------------

On this, it skipped creating cluster and install.

This https://github.com/verdaccio/charts/runs/2654454267 good reference. Here I see install going through.

@juanpicado
Copy link
Copy Markdown
Member

Fails on install, you might check the error

I have already installed this on my cluster,
Did you check the log? https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:9:90 and
https://github.com/verdaccio/charts/pull/69/checks?check_run_id=2965258840#step:8:30
Anyway, I will add small change to retrigger this.

I see this, I also triggered #70 and all is green.

Namespace 'verdaccio-xjtdpv878c' terminated.
Error: Error installing charts: Error processing charts
----------------------------------------------------------

On this, it skipped creating cluster and install.

This https://github.com/verdaccio/charts/runs/2654454267 good reference. Here I see install going through.

Yeah, something is broken, I do not why, we should wait until is green, maybe is just a hickup.

metadata:
annotations:
checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}
checksum/htpasswd-secret: {{ include (print $.Template.BasePath "/htpasswd-secret.yaml") . | sha256sum }}
Copy link
Copy Markdown
Collaborator

@rblaine95 rblaine95 Jul 2, 2021

Choose a reason for hiding this comment

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

It may be better to hash .Values.secrets.htpasswd than the secret because the htpasswd function returns different output every time you run helm upgrade even though the actual users/passwords didn't change.

checksum/htpasswd: {{ sha256sum .Values.secrets.htpasswd }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This way the checksum will only change when the actual user/passwords change and not because htpasswd returned different output for the same input for every helm upgrade

@juanpicado
Copy link
Copy Markdown
Member

I've fixed the pipeline here #70 please approve and you can rebase on top of it.

@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 2, 2021

Finally went through,
@juanpicado Thanks fixing, it. 👍🏾
@rblaine95 Thanks for reviewing. 👍🏾

@juanpicado
Copy link
Copy Markdown
Member

Just a suggestion, I am learning helm and kubernettes, maybe you can expand your PR will an example configuration so :) I can try myself locally.

@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 3, 2021

Just a suggestion, I am learning helm and kubernettes, maybe you can expand your PR will an example configuration so :) I can try myself locally.

Added an example section.

@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 7, 2021

Just a friendly reminder, I have updated everything mentioned.

Copy link
Copy Markdown
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

LGTM :) waiting @verdaccio/kubernetes experts to approve

@juanpicado juanpicado requested a review from a team July 7, 2021 10:59
Copy link
Copy Markdown
Contributor

@marekaf marekaf left a comment

Choose a reason for hiding this comment

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

lgtm

@juanpicado
Copy link
Copy Markdown
Member

@rblaine95 Can I merge?

Copy link
Copy Markdown
Collaborator

@rblaine95 rblaine95 left a comment

Choose a reason for hiding this comment

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

I'm just thinking there must be a clean way to make the secret mount and config dynamic...
However, I think that can be done in a different PR.

This PR LGTM 👍

@juanpicado juanpicado merged commit aaa72b1 into verdaccio:master Jul 8, 2021
@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 8, 2021

Thanks, @juanpicado, @marekaf and @rblaine95 .

@juanpicado
Copy link
Copy Markdown
Member

Thank all of you for keep improving the chart!

@pshanoop
Copy link
Copy Markdown
Collaborator Author

pshanoop commented Jul 8, 2021

I'm just thinking there must be a clean way to make the secret mount and config dynamic...
However, I think that can be done in a different PR.

This PR LGTM

I also feel the same, If you have some thoughts. Please put ideas on discussion thread. :)

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.

4 participants