BUILD-87: secret configmap volume mounts in builds #245
Conversation
|
/assign @adambkaplan @gabemontero |
|
/retest |
|
FYI there is auto linking of PRs to Jira items if you /retitle BUILD-87 secret configmap volume mounts in builds |
|
/retitle BUILD-87: secret configmap volume mounts in builds |
|
Course I don't have permissions to do it @coreydaley :-) |
openshift-ci
bot
changed the title
openshift-ci
bot
changed the title
|
/hold Given the complexity of the feature and the time remaining before 4.8 reaches "final freeze" period, we are going to hold this until master opens for 4.9 |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest unit |
|
@coreydaley: The
Use
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. |
|
@adambkaplan ptal |
|
/retest |
|
Test on PR launched cluster launch #245,openshift/openshift-controller-manager#183,openshift/openshift-apiserver#208,openshift/oc#843 following test cases https://url.corp.redhat.com/buildvolumes , no critical issues found. /label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
/retest |
1 similar comment
|
/retest |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
@adambkaplan @gabemontero ptal, tests are passing and it is ready for review. |
There was a problem hiding this comment.
A few comments, but no big blockers ... other than perhaps sharing some functions between builder and OCM in https://github.com/openshift/runtime-utils most likely these are just questions of mine you can answer @coreydaley and we can move on.
|
|
||
| // appends a transient mount to the map and returns an error if a duplicate | ||
| // destination is detected | ||
| func (t TransientMounts) append(mount TransientMount) error { |
There was a problem hiding this comment.
might be the first time I've seen in practice a non-struct ptr used for this .... cool :-)
There was a problem hiding this comment.
btw, the map ref is a ptr under the cover iirc correctly, hence you need nil checks when checking map variables, re: related to my comment above with the funcs off of the TransientMount struct
There was a problem hiding this comment.
might be the first time I've seen in practice a non-struct ptr used for this .... cool :-)
I'm not going to say that this is common practice, or even the correct thing to do, but it worked :)
There was a problem hiding this comment.
btw, the map ref is a ptr under the cover iirc correctly, hence you need nil checks when checking map variables, re: related to my comment above with the funcs off of the
TransientMountstruct
I believe that ok := t[mount.Destination]; equates to a nil check, as it returns false if the key does not exist. Beyond that we are really only doing a few operations here:
- Checking for duplicate keys
- Adding a key/value pair
- Outputting the map as a slice of strings constructed from the transient mount that is the value
If there is a specific place you think that I should be checking fornillet me know and I will add it.
There was a problem hiding this comment.
yeah I'm good with this one ... go ahead and mark this one resolved ... but the other thread needs to be "unresolved"
| @@ -150,3 +154,15 @@ func ParseProxyURL(proxy string) (*url.URL, error) { | |||
|
|
|||
| return proxyURL, err | |||
| } | |||
|
|
|||
| // NameForBuildVolume returns a valid pod volume name for the provided build volume name. | |||
There was a problem hiding this comment.
Did I see an exchange in a library-go PR that landed with NOT including a method like this there?
I ask because I presume (though I have not visited your PR there super recently) there is a need for such a method in openshift-controller-manager, and we'd ideally share the impl (albeit this is a simple thing).
But it may not be a given, and if so, say as much @coreydaley and we can resolve this thread ... but I'll mention it here anyway just in case.
So just let me know and we can go from there.
There was a problem hiding this comment.
we do have https://github.com/openshift/runtime-utils already for sharing build specific logic that could be a landing spot for such common methods that is specific to builds vs. library-go
There was a problem hiding this comment.
It was decided by the powers that be that this code should be duplicated between the repos instead of shared via library-go, and as such, the library-go pull request has been closed.
As for runtime-utils, all I see there is a doc.go in the build directory?
There was a problem hiding this comment.
It was decided by the powers that be that this code should be duplicated between the repos instead of shared via library-go, and as such, the library-go pull request has been closed.
ok so my recollection was correct in this case
As for
runtime-utils, all I see there is adoc.goin the build directory?
no there is also a pkg directory, with currently a "registries" subdir
technically speaking, per the readme, this repo was a common ground for https://github.com/containers related shared libs that build code would leverage, but there is no reason IMO would could not expand that a bit
@adambkaplan and @bparees are in the OWNER file
also note, that repo arose when similar utility functions were rejected by library-go
There was a problem hiding this comment.
/approve
@rolfedh @RickJWagner ptal - we need "docs-approved" and "px-approved" labels before we merge.
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
ok, employing the same
/lgtm
with
/hold
while we wait on @adambkaplan to chime in on expanding https://github.com/openshift/runtime-utils to capture these common build specific constants and funcs that are denied inclusion into openshift/library-go
if we don't use it, unhold @coreydaley
openshift-ci
bot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, coreydaley, gabemontero 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 |
|
Keeping the hold for docs/px approvals. I don't think we should expand the scope of runtime-utils to add this copied code, since it pollutes that repo's purpose. I am disappointed that our PR to library-go was rejected, since other code in that repository doesn't meet their criteria. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
https://issues.redhat.com/browse/BUILD-87