Dockerfile: Add support of dockerHubMirror - #606
stanislav-zaprudskiy wants to merge 2 commits into
Conversation
majewsky
left a comment
majewsky
left a comment
There was a problem hiding this comment.
Please explain the usecase where you have to set this manually. go-m-m already has way too many config parameters, so I am exercising extensive scrutiny for all additions.
majewsky
left a comment
majewsky
left a comment
There was a problem hiding this comment.
Ah sorry, I did not see your explanation in Slack.
Since this is only a temporary workaround, I reject the addition of a new config flag. Please hardcode the repository in question with a TODO marker to remove this once we have fixed the issue in Keppel.
0a9aa5c to
e50c5c8
Compare
There was a problem hiding this comment.
I made Dockerfile: Add support of dockerHubMirror reversible
| var AutogeneratedHeader string | ||
|
|
||
| /////////////////////////////////////////////////////////////////////////////// | ||
| // ///////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Sorry about this change, but this is a non-conformant comment. My linter keeps re-formatting it since forever whenever I work with the file, complicating further git add (that now requires -p).
| if strings.HasPrefix(cfg.Metadata.URL, "https://github.wdf.sap.corp") { | ||
| var dockerHubMirror = cfg.Dockerfile.DockerHubMirror | ||
| if dockerHubMirror == "" && cfg.Metadata.IsSAPInternalProject() { | ||
| dockerHubMirror = "keppel.eu-de-1.cloud.sap/ccloud-dockerhub-mirror/library/" |
There was a problem hiding this comment.
I'm wary to change this value for everyone (other mirrors could have other issues I haven't yet discovered). How about keeping a undocumented temporary config entry in the meantime?
03590f9 to
fa52afe
Compare
fa52afe to
d7c4af1
Compare
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
|
Should be good now, would you have a look? |
| // Deprecated: it's a temporary field that could go away w/ notice. | ||
| // | ||
| // TODO: remove when keppel issues with anonymous unauthenticated | ||
| // pulls are sorted out (ref: /archives/CE3JA3VSL/p1785758618816019) | ||
| DockerHubMirror string `yaml:"dockerHubMirror"` |
There was a problem hiding this comment.
No, please don't add this with a deprecation notice either. I still will have to hunt around tearing it out of the walls again in a few weeks. Please hardcode a match for the specific repo URLs where you need the different mirror.
2 participants
This could be useful overall (including when
cfg.Metadata.IsSAPInternalProject()istrue) in order to get around issues with default registries.