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

[JENKINS-26100] Return a map of SCM-contributed variables #16

Merged
merged 11 commits into from Jun 20, 2017

Conversation

Projects
None yet
4 participants
@abayer
Copy link
Member

commented May 1, 2017

@abayer abayer requested a review from jglick May 1, 2017

@jglick
Copy link
Member

left a comment

Looks good except for requested rename of buildEnvVars upstream.

help.html for the step should be adjusted to mention its return value.

pom.xml Outdated
@@ -38,17 +38,18 @@
</pluginRepository>
</pluginRepositories>
<properties>
<jenkins.version>1.642.3</jenkins.version>
<jenkins-core.version>2.58-20170501.171905-3</jenkins-core.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
<jenkins-war.version>2.58-20170501.171927-3</jenkins-war.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->

This comment has been minimized.

Copy link
@jglick

jglick May 1, 2017

Member

java.level8

@jglick

jglick approved these changes May 3, 2017

pom.xml Outdated
@@ -38,17 +38,19 @@
</pluginRepository>
</pluginRepositories>
<properties>
<jenkins.version>1.642.3</jenkins.version>
<jenkins-core.version>2.58-20170502.192524-8</jenkins-core.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
<jenkins-war.version>2.58-20170502.192544-8</jenkins-war.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->

This comment has been minimized.

Copy link
@jglick

This comment has been minimized.

Copy link
@jglick

jglick May 16, 2017

Member

2.60 I meant.

pom.xml Outdated
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
<version>1.15</version>
<version>2.10</version>

This comment has been minimized.

Copy link
@jglick

jglick May 16, 2017

Member

Do you not need 2.12?

This comment has been minimized.

Copy link
@abayer

abayer May 16, 2017

Author Member

Nope - this is dependent on the SCM implementation only. It won't do anything with an earlier SCM implementation (i.e., one which doesn't depend on workflow-job 2.12 and friends) but it won't error. That said, might as well bump it.

This comment has been minimized.

Copy link
@jglick

jglick May 17, 2017

Member

Do you not need to have an integration test?

This comment has been minimized.

Copy link
@jglick

jglick May 17, 2017

Member

Never mind, you do—it just does not depend on workflow-job changes.

* A step which checks out some kind of {@link SCM}, and then runs a body with any environment variables contributed
* by the {@link SCM} added to the environment.
*/
public class WithCheckoutStep extends Step implements Serializable {

This comment has been minimized.

Copy link
@jglick

jglick May 16, 2017

Member

I think we can do without this.

This comment has been minimized.

Copy link
@abayer

abayer May 16, 2017

Author Member

Okiedokie, will remove.

@@ -38,17 +38,18 @@
</pluginRepository>
</pluginRepositories>
<properties>
<jenkins.version>1.642.3</jenkins.version>
<jenkins.version>2.60</jenkins.version>

This comment has been minimized.

Copy link
@jglick

jglick May 17, 2017

Member

You will also need to patch Jenkinsfile to just buildPlugin(). And check InjectedTest—seems to be failing on CI.

This comment has been minimized.

Copy link
@abayer

abayer May 18, 2017

Author Member

Done and done.

@reviewbybees

This comment has been minimized.

Copy link

commented May 18, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick

jglick approved these changes May 18, 2017

@@ -32,10 +32,12 @@
import java.io.File;
import java.util.Iterator;
import java.util.List;
import jenkins.plugins.git.GitSampleRepoRule;

import jenkins.plugins.git.*;

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member

BTW general code style is to avoid wildcard imports except for static imports of standard test utilities.

This comment has been minimized.

Copy link
@abayer

abayer May 19, 2017

Author Member

Stupid intellij! Fixing.

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 20, 2017

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2017

@jglick Should I merge this?

@jglick

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

No, it may not be merged until jenkinsci/git-plugin#492 is released. master branch must always be releasable.

@jglick

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

Or, if you want to speed things up, @Ignore the original test for now and write a replacement using subversion 2.8.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2017

@jglick Switched to using subversion:2.8.

@jglick

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

(or mercurial 2.61 IIRC would work)

@jglick

jglick approved these changes Jun 20, 2017

@jglick jglick merged commit 2d2c718 into jenkinsci:master Jun 20, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@@ -38,17 +38,18 @@
</pluginRepository>
</pluginRepositories>
<properties>
<jenkins.version>1.642.3</jenkins.version>
<jenkins.version>2.60</jenkins.version>
<java.level>8</java.level>

This comment has been minimized.

Copy link
@i386

i386 Jul 4, 2017

🐛 Hard dependency on Jenkins 2.60 and JDK 8 means we can no longer bundle this version in Blue Ocean's aggregator :(

Are the minimums here a necessary change?

Somewhat concerned about this as the Blue Ocean ATH won't be run with all the latest Pipeline goodness. From this point onwards we have lost end to end tests for this plugin as well as workflow-job.

See jenkinsci/blueocean-plugin@d942692

This comment has been minimized.

Copy link
@abayer

abayer Jul 4, 2017

Author Member

Yes, this was required. The underlying change in core didn't go in until 2.60.

This comment has been minimized.

Copy link
@i386

i386 Jul 4, 2017

Its going to take years for people to be able to use any changes in this plugin from now onwards

picture1

This comment has been minimized.

Copy link
@abayer

abayer Jul 4, 2017

Author Member

shrug This required a change in core to work, so...

@i386 i386 referenced this pull request Jul 4, 2017

Merged

Dependency updates #1219

0 of 7 tasks complete
@jglick

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

we can no longer bundle this version in Blue Ocean's aggregator

So, don’t.

the Blue Ocean ATH won't be run with all the latest Pipeline goodness

Well that is up to your test infrastructure. jenkinsci/acceptance-test-harness by default runs the latest plugin releases compatible with the selected jenkins.war. If you choose to “pin” plugin versions, I suppose for test repeatability, then you probably want to have a parallel run (perhaps less frequent) that either uses floating versions from the update center, or pins a separate array of versions compatible with a newer LTS line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.