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

Bump pipeline and git dependencies #817

Merged
merged 2 commits into from Mar 6, 2017
Merged

Conversation

i386
Copy link
Contributor

@i386 i386 commented Feb 13, 2017

Description

@michaelneale @vivek a number of new pipeline updates are available we should integrate and test with.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
  • Ran Acceptance Test Harness against PR changes.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@reviewbybees

@i386 i386 changed the title Bump pipeline dependencies Bump pipeline and git dependencies Feb 13, 2017
@michaelneale
Copy link
Member

Still isn't happy with it - probably something else is pulling in 2.10.. perhaps hold off until workflow-api has been fixed?

@@ -302,6 +302,11 @@
<artifactId>github-organization-folder</artifactId>
<version>1.6</version>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this one is explicitly set?

@i386 i386 force-pushed the topic/dependency-update-12-02-2017 branch 2 times, most recently from 5ac1d5f to 4209a86 Compare February 21, 2017 23:39
@ghost
Copy link

ghost commented Feb 25, 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.

@michaelneale
Copy link
Member

Copy link
Member

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

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

not sure why some dependencies are added

@@ -109,7 +109,10 @@
<artifactId>unirest-java</artifactId>
<scope>test</scope>
</dependency>

<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

@i386 any reason this is here and not test scoped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scopes are inherited

Copy link
Member

Choose a reason for hiding this comment

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

nice.

Copy link
Member

Choose a reason for hiding this comment

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

so - why is this dependency here? It wasn't used before right?

@michaelneale
Copy link
Member

well looking good so far according to smoke and ATH tests.

pom.xml Outdated
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.7</version>
<version>2.12-alpha-2</version>
Copy link
Member

Choose a reason for hiding this comment

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

@i386 We're ready to take this up to 2.12 now

@i386 i386 force-pushed the topic/dependency-update-12-02-2017 branch from 605c73a to 2a164f8 Compare March 6, 2017 17:35
@i386 i386 force-pushed the topic/dependency-update-12-02-2017 branch from 2a164f8 to 9d5d00d Compare March 6, 2017 17:37
@michaelneale
Copy link
Member

michaelneale commented Mar 6, 2017

@i386 I can't tell what your latest changes are - but they have broken both unit tests and the ATH
Well if I click on the hash above - it shows me a bunch of changes I have already seen before, so I am not sure what is in #9d5d00d

@@ -17,7 +17,7 @@

<properties>
<java.level>7</java.level>
<jackson.version>2.2.3</jackson.version>
<jackson.version>2.8.7</jackson.version>
Copy link
Member

Choose a reason for hiding this comment

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

why?

@michaelneale
Copy link
Member

🐛 some failures and a lot of dependency changes that need a bit more description/work.

@michaelneale
Copy link
Member

I don't think we have the luxury of bumping deps all at once unless we have time to baby sit all the tests/ATH. wouldn't it make sense to focus on workflow-api on its own?

@michaelneale
Copy link
Member

@i386 did you squash commits here ? as I Can't see changes between today and yesterday to know what caused breakage (best to not squash when pushing to a branch for this reason).

@michaelneale
Copy link
Member

🐝 LGTM

@i386 i386 merged commit 43bce4c into master Mar 6, 2017
@michaelneale michaelneale deleted the topic/dependency-update-12-02-2017 branch March 6, 2017 23:58
@jglick
Copy link
Member

jglick commented Mar 8, 2017

Revert 45aa4f0 and fix your tests to checkout scm.

@michaelneale
Copy link
Member

michaelneale commented Mar 8, 2017

@jglick ?? commenting on long closed PRs?

EDIT: I assume you mean that changing tests to not use git but use checkout scm wont' see this problem in 2.13?

@jglick
Copy link
Member

jglick commented Mar 8, 2017

commenting on long closed PRs?

Two days! Well within the range of my review backlog, unfortunately…

you mean that changing tests to not use git but use checkout scm

No, IIUC the problem is that there are tests which create a Jenkinsfile which is just, say,

echo 'hi!'

and as of 2.13 (using the now-default lightweight mode—controllable via system property) successive builds of that branch project will not display any SCM changelog, because Jenkins is not running all the machinery of SCM.checkout and its changelog parsing—it is just grabbing the Jenkinsfile straight out of the selected revision (in the case of GitHub, via API). So the test should actually do something with the source code:

node {
  checkout scm
  echo 'hi!'
}

Now I might try to implement changelog generation even in lightweight mode—in principle the SCMFileSystem APIs make it possible, though AFAIK this code path is untested. But it would be a fair amount of work for what seems like a corner case so it is not exactly a high priority.

@michaelneale
Copy link
Member

@jglick haha yes 2 days - ancient history!

OK I will take a look at this today, seems pretty simple. If that works, makes sense. They are contrived cases. They are only "echo" so they would in theory work on windows (wishful thinking there though).

@jglick
Copy link
Member

jglick commented Mar 8, 2017

they would in theory work on windows

checkout scm should be safe on Windows too. But rather than guess, why not patch your own Jenkinsfile to run your tests on the label windows, which ci.jenkins.io supports?

@michaelneale
Copy link
Member

@jglick we aren't yet running on ci.jenkins.io - but a dogfood instance, but otherwise yes. What we are planning is some windows + browser permutations first before building js on windows (I believe it can work though, but its more of a pain than it should be with npm).

@vivek
Copy link
Collaborator

vivek commented Mar 8, 2017

@jglick yeah, we will fix the test to do that and upgrade to workflow-multibranch 2.13. thanks.

There is context to that, the upgrade to 2.13 was reverted in this PR because the test failure was recorded as regression, see https://issues.jenkins-ci.org/browse/JENKINS-42525. We will take care of it in another PR.

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