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.

Sign up for GitHub

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

Jump to bottom

Allows only HTTPS unless 'allowHttp' is true. #437

Merged
merged 13 commits into from
Jun 26, 2018
Merged

Allows only HTTPS unless 'allowHttp' is true. #437

merged 13 commits into from
Jun 26, 2018

Conversation

coollog
Copy link
Contributor

@coollog coollog commented Jun 21, 2018

Redo of #407 and regards fixing #388 .

Next PR is adding the plugin configuration allowInsecureRegistries.

@coollog coollog added this to the 0.9.1 milestone Jun 22, 2018
loosebazooka
loosebazooka reviewed Jun 26, 2018
View reviewed changes
jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryClient.java Outdated
@Nullable Authorization authorization,
String serverUrl,
String imageName,
boolean allowHttp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's something to readability when using builders or static factories here. Having to look at what a "true/false" parameter means when reading the code is not the most readable experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added static factories.

@coollog coollog mentioned this pull request Jun 26, 2018
Merged
loosebazooka
loosebazooka reviewed Jun 26, 2018
View reviewed changes
Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think I made a mistake here, registry client is internal only to jib-core? It made the code more verbose in ways I didn't realize. It might make sense to leave it how you had before (or use a builder if you want).

@coollog
Copy link
Contributor Author

coollog commented Jun 26, 2018

Changed to be a Factory class - this will help us to refactor the UserAgent configuration into this Factory class rather than setting it staticly on RegistryClient

@coollog coollog merged commit 3a8ee13 into master Jun 26, 2018
@coollog coollog deleted the allow-http branch June 26, 2018 19:13
@coollog coollog mentioned this pull request Jul 14, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loosebazooka loosebazooka loosebazooka approved these changes

@TadCordle TadCordle Awaiting requested review from TadCordle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9.1
Development

Successfully merging this pull request may close these issues.
2 participants
@coollog @loosebazooka