Skip to content
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

Merged
merged 12 commits into from
Nov 8, 2022
Merged

Add insecure repository support #1403

merged 12 commits into from
Nov 8, 2022

Conversation

fahhem
Copy link
Contributor

@fahhem fahhem commented Jan 28, 2020

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!

@fahhem
Copy link
Contributor Author

fahhem commented Jan 28, 2020

/assign @smukherj1

@fahhem
Copy link
Contributor Author

fahhem commented Jan 28, 2020

This might also address #1245

Copy link
Collaborator

@smukherj1 smukherj1 left a 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?

@jake-arkinstall
Copy link

Any update on this?

@asv
Copy link

asv commented Jan 25, 2021

@fahhem will you finish this PR or can I post mine? For our production, this is a very important feature in rules_docker.

@fahhem
Copy link
Contributor Author

fahhem commented Jan 25, 2021

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.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fahhem
To complete the pull request process, please assign smukherj1 after the PR has been reviewed.
You can assign the PR to them by writing /assign @smukherj1 in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 3, 2021
@fahhem
Copy link
Contributor Author

fahhem commented Feb 3, 2021

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.

@cfife-btig
Copy link

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.

@fahhem fahhem requested review from smukherj1 and removed request for nlopezgi, DaveGay, eytankidron and alex1545 September 24, 2022 17:50
@fahhem fahhem requested a review from gravypod as a code owner September 24, 2022 17:52
@cfife-btig
Copy link

I'm just happy to see some progress is on this MR :')

@fahhem
Copy link
Contributor Author

fahhem commented Oct 18, 2022

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.

@fahhem fahhem requested review from uhthomas and removed request for smukherj1, gravypod and pcj October 18, 2022 18:05
@fahhem fahhem requested review from smukherj1 and uhthomas and removed request for uhthomas and smukherj1 October 18, 2022 23:14
@fahhem
Copy link
Contributor Author

fahhem commented Oct 18, 2022

sorry for the noise, github's reviewer UI looks... strange. Anyone consider reviewable.io?

@fahhem
Copy link
Contributor Author

fahhem commented Oct 29, 2022

@uhthomas sorry to bother!

@cfife-btig
Copy link

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.

@uhthomas
Copy link
Collaborator

uhthomas commented Nov 3, 2022

Looks okay, we'll need to understand why CI isn't happy and then we can merge.

@fahhem
Copy link
Contributor Author

fahhem commented Nov 4, 2022

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.

@linzhp
Copy link
Collaborator

linzhp commented Nov 6, 2022

Sorry for the delay. I just fixed CI (#2178)

@fahhem
Copy link
Contributor Author

fahhem commented Nov 8, 2022

Thank you @linzhp I fixed the tests too, so this PR should be ready to go!

@linzhp linzhp merged commit 6482756 into bazelbuild:master Nov 8, 2022
@cfife-btig
Copy link

🎉🎉🎉

@fahhem fahhem deleted the add_insecurity branch November 9, 2022 02:59
@yc185050
Copy link

Any idea when will this be released? Waiting on this fix.

@fahhem
Copy link
Contributor Author

fahhem commented Jan 17, 2023

@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

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

Successfully merging this pull request may close these issues.

10 participants