Fixing default for destinationNamespace in tag step #87
Conversation
|
Replaced whitespace with spaces, so advising PR review with w=1: |
| } | ||
|
|
||
| default String getDestinationNamespace(Map<String, String> overrides) { | ||
| return getOverride(getDestinationNamespace(), overrides); |
There was a problem hiding this comment.
doesn't the doc say this falls back to the source namespace if it's empty?
There was a problem hiding this comment.
correct .... per the discussion @jupierce and I had on IRC, this needs to be
- call getOverride(getDestin....), if non-empty return
- return getNamespace(overrides)
getNamespace(overrides) should handle the fetching from NAMESPACE_ENV_VAR
There was a problem hiding this comment.
| } | ||
|
|
||
| default String getDestinationNamespace(Map<String, String> overrides) { | ||
| return getOverride(getDestinationNamespace(), overrides); |
There was a problem hiding this comment.
correct .... per the discussion @jupierce and I had on IRC, this needs to be
- call getOverride(getDestin....), if non-empty return
- return getNamespace(overrides)
getNamespace(overrides) should handle the fetching from NAMESPACE_ENV_VAR
| default String getSrcTag(Map<String, String> overrides) { | ||
| String val = getOverride(getSrcTag(), overrides); | ||
| // If there is no source namespace, default to jenkins namespace | ||
| if (val.length() == 0 && overrides != null && overrides.containsKey(NAMESPACE_ENV_VAR)) { |
There was a problem hiding this comment.
I think this was meant for getDestinationNamespace .... that tag should not be derived from the namespace env var ... that said, we can just leverage getNamespace there
|
also, in the future let's do large reformatting changes as their own commit. this is going to be tough to look back at and understand the diff. |
|
On Wednesday, October 12, 2016, Ben Parees notifications@github.com wrote:
|
|
I think the confusion arises from where the defaulting logic taking place at https://github.com/openshift/jenkins-plugin/blob/master/src/main/java/com/openshift/jenkins/plugins/pipeline/model/IOpenShiftImageTagger.java#L131 The current PR does the following:
Destination namespace precedence:
|
|
@jupierce and I looked over this at my cube. He's removing the change he made to getSrcTag, and moving defaulting logic in the |
|
Removed erroneous defaulting logic from getSrcTag. Moved actual defaulting logic from coreLogic to getDestinationNamespace for clarity. |
|
IGTM ... will hit the button after the test job completes successfully |
Found issue in public version of plugin where tagger will default to the jenkins/OpenShift namespace if destinationNamespace is not supplied. This should fix it.
ptal @gabemontero @bparees