Skip to content

Conversation

@todpunk
Copy link
Contributor

@todpunk todpunk commented Jun 23, 2020

I present this as a suggestion, and it will:

  • Update the ingress to remove the second path that conflicts in Traefik 2.x and I assume other ingress controllers that are more strict about their matching.
  • Update the deprecated apiVersion for this K8s object and leave an option for backwards compatibility for those that are on older kubernetes versions.

I am running this in production currently and it seems to work with the second ingress path disabled and the new api version. I have added comments to the values file for posterity.

Would you like anything changed on this?

@juanpicado juanpicado requested a review from a team June 23, 2020 19:13
@kav
Copy link
Collaborator

kav commented Jun 23, 2020

Any reason not to just remove the second path? I don't know that it's adding any value there. Does anyone have a use case that calls for it? nginx definitely doesn't need it and it's currently non-idomatic. I've not run across a need for a /* route for any of the ingress controllers or seen it in any other charts

@todpunk
Copy link
Contributor Author

todpunk commented Jun 23, 2020

I do not know every ingress controller. I have seen a kubernetes-dashboard chart have multiple paths: https://github.com/kubernetes/dashboard/blob/master/aio/deploy/helm-chart/kubernetes-dashboard/templates/ingress.yaml

As pointed out, it is more idiomatic to have that parsed out. I left it the way I had to maintain compatibility if anyone else is using it currently.

@todpunk
Copy link
Contributor Author

todpunk commented Jun 23, 2020

I'm going to modify this really quickly to match the dashboard chart I linked, as I think I can make that flexible for either "default" we want to carry forward. Uno momento.

@todpunk
Copy link
Contributor Author

todpunk commented Jun 23, 2020

That should be more akin to the example of the kubernetes-dashboard chart now. Paths and hosts are also separated so hosts is not necessarily required to use the ingress.

Copy link
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.

Thanks @todpunk 💯

@juanpicado juanpicado merged commit 432d3b2 into verdaccio:master Jun 24, 2020
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