search

openshift / jenkins-plugin

Apache License 2.0
81 stars 50 forks source link

Fixing default for destinationNamespace in tag step #87

Closed jupierce closed 8 years ago

jupierce commented 8 years ago

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

jupierce commented 8 years ago

Replaced whitespace with spaces, so advising PR review with w=1: https://github.com/openshift/jenkins-plugin/pull/87/files?w=1

bparees commented 8 years ago

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.

gabemontero commented 8 years ago

On Wednesday, October 12, 2016, Ben Parees notifications@github.com wrote:

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.

My intention had be to convert all the files to space only once this sprint was done and the amount of changes settled down.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/jenkins-plugin/pull/87#issuecomment-253374427, or mute the thread https://github.com/notifications/unsubscribe-auth/ADbadMS5imRNVNTmx7Y72pUdr4zoYUqIks5qzXWsgaJpZM4KVPMI .

jupierce commented 8 years ago

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: Source namespace order of precedence:

  1. The overridden value for the sourceNamespace field
  2. The value of the sourceNamespace field
  3. The namespace hosting Jenkins

Destination namespace precedence:

  1. The override value for the destinationNamespace field
  2. The destinationNamespace field 3 The source namespace
gabemontero commented 8 years ago

@jupierce and I looked over this at my cube. He's removing the change he made to getSrcTag, and moving defaulting logic in the coreLogic method to getDestinationNamespace.

jupierce commented 8 years ago

Removed erroneous defaulting logic from getSrcTag. Moved actual defaulting logic from coreLogic to getDestinationNamespace for clarity.

gabemontero commented 8 years ago

IGTM ... will hit the button after the test job completes successfully