search

openshift / jenkins-plugin

Apache License 2.0
81 stars 50 forks source link

Merge time units #80

Closed jupierce closed 8 years ago

jupierce commented 8 years ago

Merging #65 on top of #78

ptal @oatmealraisin @bparees @gabemontero

jupierce commented 8 years ago

Will finish dsl extension merge on Monday.

gabemontero commented 8 years ago

looks great so far !

oatmealraisin commented 8 years ago

@jupierce lgtm, I'm going to pull and test some stuff, thanks!

Edit: It looks like not everything is finished yet, I'll wait for the go-ahead on Monday! Have a nice weekend

gabemontero commented 8 years ago

Yeah and we also have intellij vs eclipse going on here as well. We can confer at our next team meeting. Maybe we need to go to make all tabs spaces.

On Sunday, October 9, 2016, Ben Parees notifications@github.com wrote:

@bparees requested changes on this pull request.

couple minor formatting comments, otherwise lgtm so far.

In src/main/java/com/openshift/jenkins/plugins/pipeline/ TimedOpenShiftBasePostAction.java https://github.com/openshift/jenkins-plugin/pull/80#pullrequestreview-3435353 :

@@ -8,14 +8,20 @@ public abstract class TimedOpenShiftBasePostAction extends OpenShiftBasePostAction implements SimpleBuildStep, Serializable, ITimedOpenShiftPlugin {

 protected final String waitTime;
  • protected final String waitUnit;

fix tabbing

In src/main/java/com/openshift/jenkins/plugins/pipeline/ TimedOpenShiftBaseStep.java https://github.com/openshift/jenkins-plugin/pull/80#pullrequestreview-3435353 :

    super( apiURL, namespace, authToken, verbose );
  this.waitTime = waitTime;
  • this.waitUnit = waitUnit;

tabbing

In src/main/java/com/openshift/jenkins/plugins/pipeline/ TimedOpenShiftBaseStep.java https://github.com/openshift/jenkins-plugin/pull/80#pullrequestreview-3435353 :

    super( apiURL, namespace, authToken, verbose );
  this.waitTime = waitTime;
  • this.waitUnit = waitUnit;

@gabemontero https://github.com/gabemontero perhaps we should check in an eclipse format definition to the repo? or at least describe one in the readme (tabs vs spaces, tab-stop settings, braces on same line or next

line, etc)

In src/main/java/com/openshift/jenkins/plugins/pipeline/ model/ITimedOpenShiftPlugin.java https://github.com/openshift/jenkins-plugin/pull/80#pullrequestreview-3435353 :

default long getDefaultWaitTime() {
  return GlobalConfig.UBER_DEFAULT_WAIT;

}

String getWaitTime();

  • String getWaitUnit();

tabbing

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

jupierce commented 8 years ago

There is a mixture of 4space/tab throughout the files (even intra-file). Based on Gabe's comment, I will check in with 4space for now.

jupierce commented 8 years ago

Sorry for the delay. I've found a bug in the original handling of global timeouts (i.e the currently public version of the jenkins plugin). The DSL implementations will only receive the configured global timeouts IFF the descriptor associated the pipeline build step is configured before the DSL step runs. To recreate:

I figured this was the right time to fix it since I'm in the code.

gabemontero commented 8 years ago

On Mon, Oct 10, 2016 at 1:34 PM, Justin Pierce notifications@github.com wrote:

Sorry for the delay. I've found a bug in the original handling of global timeouts (i.e the currently public version). The DSL implementations will only receive the configured global timeouts IFF the descriptor associated the pipeline build step is configured before the DSL step runs.

Good catch! Truth be told, this is not the first time the sequence of something only getting set if you enter the configure screen has bitten me.

To recreate:

  • Configure the global wait time for builds to 1ms
  • Create a pipeline job X with openshiftBuild DSL and do not specify timeout (set to empty string or do not specify at all).
  • Run X
    • It should fail quickly because the 1ms timeout expires.
  • Restart the Jenkins JVM (http://...jenkinsurl.../restart)
  • Directly after logging in, Run X
    • It will not fail and will use the static default timeout associated with builds.
    • This failure to use the configured timeout will persist until you save global configuration again.

I figured this was the right time to fix it since I'm in the code.

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

gabemontero commented 8 years ago

Update: the port of @oatmealraisin 's time units change looks goo to me as well.

We've been discussing the format situation a bit on IRC. I'm not going to halt this PR on that resolution. Once we've decided (probably during a scrum) I'll go in and clean up all the java with the form we decide upon.

Otherwise, holding on merge to give @jupierce more time with the DSL difficulties around global config. If the fix becomes a rabbit hole though, we'll punt on handling in this PR and merge this.

jupierce commented 8 years ago

All changes have been made to be backwards compatible.

jupierce commented 8 years ago

To focus in on non-formatting changes, use the ?w=1 parameter to ignore whitespace: https://github.com/openshift/jenkins-plugin/pull/80/files?w=1