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 portus chart #1284

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@so0k
Contributor

so0k commented Jun 12, 2017

Add Chart for a docker registry with access control managed by Portus

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Jun 12, 2017

Contributor

Hi @so0k. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Contributor

k8s-ci-robot commented Jun 12, 2017

Hi @so0k. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Jun 27, 2017

Contributor

any chance someone takes a look at this? @prydonius ?

Contributor

so0k commented Jun 27, 2017

any chance someone takes a look at this? @prydonius ?

@prydonius

This comment has been minimized.

Show comment
Hide comment
@prydonius

prydonius Jun 29, 2017

Member

@so0k any reason you haven't added a conditional dependency on MariaDB and Minio? I think that would be really useful to get this running out of the box.

@k8s-bot ok to test

Member

prydonius commented Jun 29, 2017

@so0k any reason you haven't added a conditional dependency on MariaDB and Minio? I think that would be really useful to get this running out of the box.

@k8s-bot ok to test

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Jun 29, 2017

Contributor

Good point, I'll try to add that - thanks

Contributor

so0k commented Jun 29, 2017

Good point, I'll try to add that - thanks

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Jun 29, 2017

Contributor

@so0k: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-charts-e2e 3160032 link /test pull-charts-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Contributor

k8s-ci-robot commented Jun 29, 2017

@so0k: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-charts-e2e 3160032 link /test pull-charts-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@viglesiasce

This comment has been minimized.

Show comment
Hide comment
@viglesiasce

viglesiasce Jul 17, 2017

Contributor

Marking as stale

Contributor

viglesiasce commented Jul 17, 2017

Marking as stale

@viglesiasce viglesiasce added the stale label Jul 17, 2017

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Jul 17, 2017

Contributor

Didn't have time yet to add conditional requirements, although it could be done in a subsequent PR?

Contributor

so0k commented Jul 17, 2017

Didn't have time yet to add conditional requirements, although it could be done in a subsequent PR?

@viglesiasce

This comment has been minimized.

Show comment
Hide comment
@viglesiasce

viglesiasce Jul 18, 2017

Contributor

@so0k we'd really prefer to have things that can work out of the box have sane defaults before merging. I may be able to add this sometime this week.

Contributor

viglesiasce commented Jul 18, 2017

@so0k we'd really prefer to have things that can work out of the box have sane defaults before merging. I may be able to add this sometime this week.

@unguiculus

Are you going to keep come up with an update? Also, please used best practices for standard labels. Especially, the app label should use name instead of fullname.

See https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/labels.md.

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Jul 31, 2017

Contributor

Ok, I had some higher priorities as the chart works as it is for us, but I will be able to complete this over the next weekend

Contributor

so0k commented Jul 31, 2017

Ok, I had some higher priorities as the chart works as it is for us, but I will be able to complete this over the next weekend

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Aug 6, 2017

Contributor

@prydonius / @viglesiasce - I've added conditional support for Minio and Mariadb

Note: a vanilla k8s cluster will need ingress controller with TLS...

@unguiculus - I'm confused regarding app label as I try to refer to several other stable charts and I don't see that best practice applied anywhere?
i.e:

most seem to use fullname template.

Also, using name template doesn't make sense to me. selector queries will overlap if pods are not labeled with release name in app label if we release the same chart multiple times in the same cluster & namespace

I'll look at changing this if you can clarify how multiple releases of same chart will no cause issues.

Contributor

so0k commented Aug 6, 2017

@prydonius / @viglesiasce - I've added conditional support for Minio and Mariadb

Note: a vanilla k8s cluster will need ingress controller with TLS...

@unguiculus - I'm confused regarding app label as I try to refer to several other stable charts and I don't see that best practice applied anywhere?
i.e:

most seem to use fullname template.

Also, using name template doesn't make sense to me. selector queries will overlap if pods are not labeled with release name in app label if we release the same chart multiple times in the same cluster & namespace

I'll look at changing this if you can clarify how multiple releases of same chart will no cause issues.

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Aug 7, 2017

Contributor

I guess for selectors we use app as well as release labels.. so we would be able to use template "name" for the app label... 🤔

Contributor

so0k commented Aug 7, 2017

I guess for selectors we use app as well as release labels.. so we would be able to use template "name" for the app label... 🤔

@unguiculus

This comment has been minimized.

Show comment
Hide comment
@unguiculus

unguiculus Aug 7, 2017

Member

@so0k Best practices have not been set in stone from day one. They evolve over time. Latest best practices are reflected in the chart examples (https://github.com/kubernetes/helm/tree/master/docs/examples) and in the experience you get when creating a new chart with helm createwith Helm 2.5+. And yes, most notably, we've started recommending {{ template "name" . }} for app labels which will require the release label as additional selector.

Member

unguiculus commented Aug 7, 2017

@so0k Best practices have not been set in stone from day one. They evolve over time. Latest best practices are reflected in the chart examples (https://github.com/kubernetes/helm/tree/master/docs/examples) and in the experience you get when creating a new chart with helm createwith Helm 2.5+. And yes, most notably, we've started recommending {{ template "name" . }} for app labels which will require the release label as additional selector.

@unguiculus unguiculus removed the stale label Aug 7, 2017

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Aug 9, 2017

Contributor

I'll try to change the labels in the next weekend

Contributor

so0k commented Aug 9, 2017

I'll try to change the labels in the next weekend

@unguiculus unguiculus added the stale label Aug 30, 2017

@unguiculus

This comment has been minimized.

Show comment
Hide comment
@unguiculus

unguiculus Sep 9, 2017

Member

Closing as stale. Feel free to reopen when/if you decide to work on it again.

Member

unguiculus commented Sep 9, 2017

Closing as stale. Feel free to reopen when/if you decide to work on it again.

@unguiculus unguiculus closed this Sep 9, 2017

@so0k so0k referenced this pull request Oct 29, 2017

Closed

[WIP] Added a Kubernetes example #1311

0 of 2 tasks complete
@rendhalver

This comment has been minimized.

Show comment
Hide comment
@rendhalver

rendhalver Nov 16, 2017

Collaborator

@so0k Do you mind if I use this as inspiration for another PR to get this working?
Or optionally do need a hand to get this one working?
We are wanting to use Portus in our environment so we will have somewhere to test it out.

Collaborator

rendhalver commented Nov 16, 2017

@so0k Do you mind if I use this as inspiration for another PR to get this working?
Or optionally do need a hand to get this one working?
We are wanting to use Portus in our environment so we will have somewhere to test it out.

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Nov 16, 2017

Contributor

@rendhalver yes please - we are currently moving to vmware/harbor

Contributor

so0k commented Nov 16, 2017

@rendhalver yes please - we are currently moving to vmware/harbor

@rendhalver

This comment has been minimized.

Show comment
Hide comment
@rendhalver

rendhalver Nov 16, 2017

Collaborator

Yes to submitting a new PR or reopening this one @so0k?

Collaborator

rendhalver commented Nov 16, 2017

Yes to submitting a new PR or reopening this one @so0k?

@so0k

This comment has been minimized.

Show comment
Hide comment
@so0k

so0k Nov 16, 2017

Contributor

you can submit a new PR please

Contributor

so0k commented Nov 16, 2017

you can submit a new PR please

@rendhalver

This comment has been minimized.

Show comment
Hide comment
@rendhalver

rendhalver Nov 16, 2017

Collaborator

Ok cool I will get that setup soon.
I will link it to this one so people know where it comes from.

Thanks for giving me a good start to getting it working.

Collaborator

rendhalver commented Nov 16, 2017

Ok cool I will get that setup soon.
I will link it to this one so people know where it comes from.

Thanks for giving me a good start to getting it working.

@rendhalver rendhalver referenced this pull request Nov 16, 2017

Open

Add portus chart #2766

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment