added option to authenticate with registry #88
Conversation
homestead-prov/Dockerfile
Outdated
| @@ -1,7 +1,7 @@ | |||
| FROM clearwater/base | |||
| MAINTAINER maintainers@projectclearwater.org | |||
|
|
|||
| RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --force-yes homestead-prov clearwater-prov-tools | |||
| RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --force-yes homestead homestead-prov clearwater-prov-tools | |||
There was a problem hiding this comment.
I think this has probably come in from a different set of changes. Would be good to revert this change here to keep the PR clean
There was a problem hiding this comment.
Ah woops. Yes that's my mistake for not keeping my repos clean and separated. I'll fix that.
kubernetes/k8s-gencfg
Outdated
| @@ -12,7 +12,15 @@ def parse_file(args, input_file, output_file): | |||
| with open(input_file) as file: | |||
| input_data = file.read() | |||
|
|
|||
| output_data = input_data.replace("{{IMAGE_PATH}}", args.image_path).replace("{{IMAGE_TAG}}", args.image_tag) | |||
| data = input_data.replace("{{IMAGE_PATH}}", args.image_path) | |||
There was a problem hiding this comment.
Any particular reason for splitting this? I think we'd probably prefer to keep this as a single line given the templating is also a single line to change. Helps to keep the concepts in sync
There was a problem hiding this comment.
I just thought that putting it in a single line leads to a line which is too long, which I consider bad style. I'm happy to change it to one line if you want.
kubernetes/k8s-gencfg
Outdated
|
|
||
| # this argument is optional | ||
| # ~ means null in yaml | ||
| output_data = data.replace("{{IMAGE_SECRET}}", args.image_secret if (args.image_secret != None) else "~") |
There was a problem hiding this comment.
I'll probably try this out a bit on our rigs, to make sure it behaves as i'm expecting, but can you give a summary of any testing/checking you've done here on if there isn't a secret provided?
There was a problem hiding this comment.
I haven't done proper testing of the no secret case, because I don't have a public repo. I did try running it on my private repo, and it seemed to parse fine and then give an authentication failed error.
Today I created a public docker hub repo. I'll try on that when I get a chance.
There was a problem hiding this comment.
Yes it does work with no authentication. I just tried.
kubernetes/k8s-gencfg
Outdated
| output_data = data.replace("{{IMAGE_SECRET}}", args.image_secret if (args.image_secret != None) else "~") | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Would be nice to clean up the unnecessary blank lines here
There was a problem hiding this comment.
Yep I'll fix that.
|
Ok @johadalin is that all good to merge now? (Pending your own tests with no authentication) |
|
Looks like this all works fine on my side, and i like the way you've formatted the templating part now 👍 From the tech side i think this is all good to merge. As i commented on a separate pull request though, we need to sort out the contributors agreement before we're able to merge this down to master. Legal licencing stuff is always a bit of a pain, but if you email the address i pointed at (linked at http://www.projectclearwater.org/community/), we should hopefully be able to sort that out quickly, and then can merge this down. Thanks again for pushing these changes over :) nice addition to the setup |
|
Yep, I've already sent an email to that address. I'm just awaiting the reply. |
|
Hey @mlda065 . Any further news on the agreement? I believe you should have got some stuff sent over; both corporate and individual ones in this case, right? |
|
I just submitted the signed agreements via email a minute ago. |
|
Excellent, all confirmed on this side too, so i'm going to go ahead and merge it down 👍 |
If you use a private container registry, you need to add a parameter to each yaml file. Doing that manually is a pain, so I added it to
k8s-gencfg.Since we're not using a full template library, I found it easiest to leave the new parameter in the yaml files even if you don't need it, and just set it to
~(yaml null).I don't have a public registry to test with, so it would be good if someone else can double check that
~works when you don't need to authenticate. It is valid yaml, so I expect it to work.