Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add organization folder support #12

Merged
merged 2 commits into from
Sep 12, 2019

Conversation

coreyjonoliver
Copy link

No description provided.

Copy link

@nickolashkraus nickolashkraus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

}

/**
* Get the current setting for whether or not to use a groovy sandbox.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Groovy (to match method comment for setUseSandbox)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for @return

@coreyjonoliver
Copy link
Author

@samrocketman Do you mind taking the opportunity to review this when you have the chance? Much appreciated!

@shamil
Copy link

shamil commented Mar 26, 2019

Really useful feature, hope will be merged soon!

@samrocketman
Copy link
Member

Hello, I was moving cross country for a couple months and was severely AFK from volunteer work. I'm starting to get back into the groove of things and will take a look at this.

Copy link
Member

@samrocketman samrocketman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit hard to tell what has changed making this more challenging to review. If you don't mind:

Please break this up into two commits.

  1. Commit where you copied code from other classes unmodified.
  2. Commit where you update the code in the classes you copied.

It makes it a little more clear that way. If you don't want to that's fine I'll still take a look as best I can.

pom.xml Outdated
@@ -37,7 +37,7 @@ THE SOFTWARE.
<java.level>8</java.level>
</properties>
<artifactId>pipeline-multibranch-defaults</artifactId>
<version>2.1-SNAPSHOT</version>
<version>2.2-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to bump the snapshot version. It will be auto-bumped upon next plugin release.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I’ve been away a few months while moving cross country. I am back now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two commits is much more clear and makes it easier to review. Thanks for taking the time to do it.

@coreyjonoliver coreyjonoliver force-pushed the add-organization-folder-support branch from 2f9efbf to 33d5028 Compare April 4, 2019 16:26
@coreyjonoliver
Copy link
Author

@samrocketman Split my work into two commits, as suggested.

@samrocketman samrocketman self-assigned this Apr 12, 2019
@nickolashkraus
Copy link

@samrocketman Mind giving this a review when you get a chance?

@caitlinelfring
Copy link

Really looking forward to this being released! Any idea of the timeline?

@coreyjonoliver
Copy link
Author

Just a friendly bump. @samrocketman do you mind taking another look at this?

@coreyjonoliver
Copy link
Author

@samrocketman sorry to sound like a broken record, but could you give this another review?

@bjulian5
Copy link

Bump!

@samrocketman
Copy link
Member

Thanks @coreyjonoliver for updating. I'll take a look at this again and test locally.

Copy link
Member

@samrocketman samrocketman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good I'm going to test it locally by configuring my own organization.

@coreyjonoliver
Copy link
Author

Great news! Thanks for taking the time to look it over @samrocketman.

@samrocketman
Copy link
Member

I'll be able to finish this up today. Apologies for the delay. I went on a bit of a hiatus from FOSS development because moving cross country kind of drained my motivation for a lot of things. Switching social circles and other non-visible things. I really appreciate your patience with me as a maintainer.

samrocketman added a commit that referenced this pull request Sep 12, 2019
@samrocketman samrocketman merged commit 33d5028 into jenkinsci:master Sep 12, 2019
@samrocketman
Copy link
Member

Successfully tested. This feature has been released and should be available in the update center within about 8 hours. If you want to try it now you can download https://repo.jenkins-ci.org/releases/org/jenkins-ci/plugins/pipeline-multibranch-defaults/2.1/pipeline-multibranch-defaults-2.1.hpi

@jetersen
Copy link
Member

jetersen commented Oct 22, 2019

Thanks @coreyjonoliver for your contribution!
This is super awesome exactly what I was looking for!
😍
This should be mentioned in the readme! I'll send a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants