Bug 1745192: Move registries.conf editing into a subpackage #1087
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
@runcom @umohnani8 @gabemontero PTAL. |
|
@mtrmac this is for 4.3 correct? |
|
@kikisdeliveryservice No, openshift/openshift-controller-manager#19 needs to happen in 4.2 ; this is to avoid duplicating all of that code in two places. We are still looking for the best place for the newly created subpackage to exist, MCO is not ideal. |
pkg/registries/topo.go
Outdated
| @@ -1,4 +1,4 @@ | |||
| package containerruntimeconfig | |||
| package registries | |||
There was a problem hiding this comment.
can this pkg be under pkg/controller/container-runtime-config?
There was a problem hiding this comment.
Sure; I moved it up here to make it easier to find / read for out-of-repo consumers (who don’t care about the M-C-controller much).
Ideally we would want to move it somewhere out of the 2 (3? 4?) known users into some shared infrastructure repo.
If it is going to stay in MCO, I really don’t care where.
There was a problem hiding this comment.
+1 on runcom's comment
There was a problem hiding this comment.
awesome, thank you!
|
@mtrmac @kikisdeliveryservice - @bparees and I talked with @cuppett at lunch today and there is a POSSIBILITY that the OCM changes that would consume this could come in post 4.2, with some sort of caveat expressed wrt disconnected support I think ultimately a common conversation should occur between us, @adambkaplan and @umohnani8 on whether this is a 4.2 blocker. |
|
(Oh, and I forgot, just in case: See the individual commits to see what changed.) |
mtrmac
force-pushed
the
registries.conf-subpackage
branch
from
d8866b9
to
b318fda
Compare
mtrmac
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
…ntime-config to ../registries Copy the updateRegistriesConfig function and its helpers, and the corresponding tests, to pkg/controller/container-runtime-config/registries . Nothing at all is changed about the code, to make actual edits easy to see in the future commits. Notably this includes the updateRegistriesConfig function being package-private, so there isn't even a way to call the added code. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…egistriesConf ... to be more descriptive. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead of receiving and returning a byte stream, edit a sysregistriesv2.V2RegistriesConf in place. This - makes it much easier for the caller to use a Go-constructed configuration as a starting point - removes a dependency on the TOML encoder from the package (but not from its tests) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…runtime-config Finally, remove the now redundant code in pkg/controller/containers-runtime-config; have updatedRegistriesConfig only decode, call registries.EditRegistriesConfig, and encode the result. TestUpdateRegistriesConfig was left unmodified, although it is now mostly redundant with the tests in pkg/controller/containers-runtime-config/registries . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
force-pushed
the
registries.conf-subpackage
branch
from
b318fda
to
af7211a
Compare
Thanks @gabemontero for explaining, please keep us updated on your plans for this. |
|
Filed openshift/library-go#513 for the library-go inclusion; that would replace this PR. |
|
|
||
| mirrorSets, err := mergedMirrorSets(icspRules) | ||
| if err != nil { | ||
| if err := registries.EditRegistriesConfig(&tomlConf, internalInsecure, internalBlocked, icspRules); err != nil { |
There was a problem hiding this comment.
hey @mtrmac I'm still working it, but if the deps update for containers/image continues to cause conflicts in the OCM, I might need an additional version of this method that
a) does not need a *sysregistriesv2.V2RegistriesConf param
b) also takes a []string parm for setting UnqualifiedSearchRegistries
c) returns a string representation of the sysregistriesv2.V2RegistriesConf
I wouldn't be entirely surprised either if removing the need for pulling in the later version of containers/image facilitates inclusion in library-go
thoughts?
There was a problem hiding this comment.
so in case I was not clear, have this method as you have, plus the new proposed methods with the modifications I mentioned
There was a problem hiding this comment.
I would be very surprised if that helps; a library-go subpackage with that modified API would pull in exactly the same containers/image, and other, dependencies, as the code calling it does right now, there would just be one layer of indirection.
The effect would basically be only that library-go would always pull in the TOML encoder/decoder as well (which makes no difference for the two currently contemplated consumers, but removes a bit of flexibility for possible future uses).
One thing that could possibly actually help is fixing containers/image#692 ; my problems with Glide in the library-go PR manifested in one of the compression implementations (e.g. via github.com/klauspost/compress/snappy/testdata/alice29.txt ).
But that would help with either of the APIs, and might not be essential — I was able to make Glide work with that one package using git add --force, at least.
There was a problem hiding this comment.
yeah quite possibly ... didn't think it through
that said, I got verify-deps to succeed in the OCM prototype PR .... https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_openshift-controller-manager/19/pull-ci-openshift-openshift-controller-manager-master-verify-deps/39
|
I've sent out an email to the interested parties on getting to a decision on merging this PR and then rebasing openshift/openshift-controller-manager#19 off of this |
|
LGTM |
mtrmac
changed the title
we need a BZ explaining why we need such PR |
|
The BZ for this is https://bugzilla.redhat.com/show_bug.cgi?id=1745192. Well it requires openshift/openshift-controller-manager#19 as well to be fully fixed. |
runcom
changed the title
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@mtrmac: This pull request references Bugzilla bug 1745192, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/approve will leave to @kikisdeliveryservice one last review |
openshift-ci-robot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kikisdeliveryservice, mtrmac, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/skip |
(WIP because this includes unmerged #1082 .)- What I did
Turned
updateRegistriesConfigfrompkg/controller/container-runtime-configintopkg/registries.EditRegistriesConfigin a separate subpackage, with minimal dependenciesto be possible to call it from other registries.
This is in preparation for possibly moving it out of this repository into library-go entirely, see openshift/openshift-controller-manager#19 for the whole discussion.
Moving it only into a subpackage in this repo is clearly non-disruptive, and it allows reviewing the proposed API before moving it.
The API deals with Go structures only, forcing the callers to do their own TOML encoding/decoding. This is to make it easier for the openshift-controller-manager code to start with a Go-only data structure without having to encode it just to submit it to the edit function.
- How to verify it
Existing unit tests.
- Description for the changelog
N/A