-
Notifications
You must be signed in to change notification settings - Fork 691
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 insecure repository support #1403
Conversation
/assign @smukherj1 |
This might also address #1245 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Do you mind adding a test here that exercises the new insecure_repository attribute?
Any update on this? |
@fahhem will you finish this PR or can I post mine? For our production, this is a very important feature in rules_docker. |
I don't have a publicly-available insecure repository available to add a test against. If you have a better PR, go ahead and post it, I'm not attached to this PR in any way. I was simply sharing what I had to do to get a on-machine docker repository to work. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fahhem The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I've rebased onto master. I don't know the test infra here, so I added a "test" that is probably wrong. 🤷 if anyone else has a better test or at least a suggestion on how to write one, I'm happy to swap mine out for it. |
We are very interested in having this merged in. We are also dependent on a registry which is insecure and are having to write lots of workarounds without this change. |
I'm just happy to see some progress is on this MR :') |
Same here :D Thanks @uhthomas, I ran gofmt and it had a couple more changes than around my code, but it was all whitespace stuff. |
sorry for the noise, github's reviewer UI looks... strange. Anyone consider reviewable.io? |
@uhthomas sorry to bother! |
I don't mean to keep pecking either. But my team is also really interested in getting this merged in as well. I know that the CI is still being blocked but it sorta sounds like it just might be some sort of noise and not anything legitimate. Although of course I also have no visibility. |
Looks okay, we'll need to understand why CI isn't happy and then we can merge. |
All PR's are blocked until CI is fixed? Is anyone at bazel working on fixing the CI? It's a lot easier to fix buildkite stuff when you have access to the servers, but I can take a stab at it still. |
Sorry for the delay. I just fixed CI (#2178) |
Thank you @linzhp I fixed the tests too, so this PR should be ready to go! |
🎉🎉🎉 |
Any idea when will this be released? Waiting on this fix. |
@yc185050 I recommend opening a new issue to request a release. Personally, I'm using master at that merged commit as my company's release while we wait for an official release |
Local registries, such as microk8s, don't have SSL setup.
This PR just adds the ability to specify to push to an insecure repository. I'm not sure if that's the intended design pattern for this, so I'm open to suggestions!