Skip to content

Add a Jenkinsfile so https://ci.jenkins.io/ will build us.#76

Merged
jswager merged 1 commit intojenkinsci:masterfrom
pjdarton:master
Jul 3, 2017
Merged

Add a Jenkinsfile so https://ci.jenkins.io/ will build us.#76
jswager merged 1 commit intojenkinsci:masterfrom
pjdarton:master

Conversation

@pjdarton
Copy link
Copy Markdown
Member

As per advice from #jenkins irc channel, we need a "Jenkinsfile" in order for the automatic builds to happen on the Jenkins CI. Until that is done, we don't get automatic builds of pull requests.
FYI https://jenkins.ci.cloudbees.com/job/plugins/job/vsphere-cloud-plugin/ is now "disabled" with the comment "DEPRECATED All jobs should be moved to https://ci.jenkins.io", ci.jenkins.io requires a Jenkinsfile, and this one is as simple as they get.

As per advice from #jenkins irc channel, we need a "Jenkinsfile"
in order for the automatic builds to happen on the Jenkins CI.
Until that is done, we don't get automatic builds of pull requests.
@pjdarton
Copy link
Copy Markdown
Member Author

@jswager @elordahl The Jenkins-on-Jenkins CI infrastructure has changed and we now need a "Jenkinsfile" in order to get the automatic building of pull requests etc that we used to get from cloudbees.

I asked on http://webchat.freenode.net/?channels=jenkins regarding how to do this, was told (https://botbot.me/freenode/jenkins/2017-06-28/?msg=87886425) what to do, and this PR was the result.
The theory is that, after this is merged into the main code, and after a few minutes delay, we should get a new build happening in https://ci.jenkins.io/job/Plugins/

Can we get this merged quickly?

@elordahl
Copy link
Copy Markdown
Contributor

I use Jenkinsfile in many other builds, but am not familiar with the buildPlugin() functionality. I'd say merge it to see if it builds 👍 then we can rebase other PRs on top.

@jswager Is it possible to merge to master without releasing?

@pjdarton
Copy link
Copy Markdown
Member Author

I think the short answer is "Yes it is possible to merge to master without releasing".

My understanding is that, at the very least, doing a release requires modifications to be made to the pom.xml.
Until those are made, you get new xxx-SNAPSHOT builds which are merely "release candidate" builds and are not releases.
e.g. Normally once ends up merging a number of PRs into master and it's only once we're happy that someone edits the pom.xml to say "this is version X.Y" instead of "X.Y-SNAPSHOT".

i.e. There may be additional steps involved in doing a release (that I'm unfamiliar with), but I'm pretty sure you don't get a release just by merging into the master.

@elordahl
Copy link
Copy Markdown
Contributor

elordahl commented Jul 3, 2017

hey @jswager, can you merge this to unblock automated builds?

@pjdarton
Copy link
Copy Markdown
Member Author

pjdarton commented Jul 3, 2017

@elordahl If the other PRs we have outstanding (since late January), Jason may have been unavailable for some time - this doesn't look recent. If you have any other means of contacting him, please try them...

I did some research and I've found that there is a process for "rescuing" a plugin where the primary developer(s) cease their involvement:

In this case, it's not dormant, we're just missing the one person who has commit access - I'm pretty sure that your voice (as an official maintainer of the plugin) could speed up the process of obtaining commit access...

Question is, would you be willing to take over? Or do you want to retain your "no commit access" status?

Note: it's important to include @jswager in all correspondence to ensure that it isn't viewed as a "secret takeover" and that we "Trying insistently" to contact Jason. If he's been offline a while then I guess we're going to fill his email inbox with this discussion...

@jswager
Copy link
Copy Markdown
Member

jswager commented Jul 3, 2017

Augh - all the notifications have been junked. Just saw this when I check things out in a while.

Yeah, I can't spend much time on this anymore. Lack both the time and testing resources to validate much of anything. Even lost my release environment, so not even sure I can push releases.

Are there volunteers to take over the project?

@jswager jswager merged commit a3c6f5f into jenkinsci:master Jul 3, 2017
@pjdarton
Copy link
Copy Markdown
Member Author

pjdarton commented Jul 4, 2017

I'm willing to put in some effort in the short term, at least enough to clear the current backlog of unmerged PRs to the point of the next release. I can't necessarily commit to anything long term (the moment I have what we need at work, my work priorities will shift away from this), but I can certainly help out for a bit. Longer term, we could just state (on the Jenkins dev forums) that the plugin was up for adoption - there's a fair few plugins which are so stable that they only ever have "part time" maintainers. Hopefully we're getting close to that with this plugin ;-)

FYI I do have a dev/test environment, and I'm no longer entirely clueless with git, but my dev/test environment is just one possible vSphere deployment - I'm aware that they can be configured very differently and testing on one doesn't prove stuff works on all of them...

Note: if I ended up doing the next release, I would really appreciate some hand-holding - it's one thing to RTFM, it's another thing to do it...

@elordahl
Copy link
Copy Markdown
Contributor

elordahl commented Jul 4, 2017

I'd recommend closing #66 and merging 71-78, which will leave four open PRs. If none of the other three PRs can be reviewed quickly, then @pjdarton can test the merged commits with a local build for a few days. If successful, release.

@pjdarton
Copy link
Copy Markdown
Member Author

pjdarton commented Jul 6, 2017

@elordahl @jswager I'd be up for that.
The blocking issue at present is that the only person with write-access (which is needed to merge a PR into the main code) is Jason.

  • Jason - if you don't have time for this, can you please grant write-access to Eric and/or myself?

We can then work our way through the PRs, merging as we go, until we end up with a release-candidate. Once we have a release candidate, we can test it. I'll try it out at work and, if #72 is any indication, @phoewass may be willing to try it too.
Once we're happy that it's stable, we can release it.

@jswager
Copy link
Copy Markdown
Member

jswager commented Jul 6, 2017

@pjdarton @elordahl - I'm trying to get the two of you access. Using this: https://wiki.jenkins.io/display/JENKINS/Adopt+a+Plugin

I have your GitHub IDs, but need your "Jenkins infrastructure account id" too. If you could send them via GitHub or direct email, that would be really helpful!

@phoewass
Copy link
Copy Markdown

phoewass commented Jul 6, 2017

Awesome! I can definitely help you guys in various ways.
I can have some spare time to contribute/test as my organization is using this plugin.

@pjdarton
Copy link
Copy Markdown
Member Author

pjdarton commented Jul 7, 2017

@jswager I'm known as "pjdarton" on in the Jenkins CI system too (and on gmail.com for that matter).
...and in the rare cases I log in to the Jenkins IRC channel, I log in as "pjdarton" there too ;-)

@pjdarton
Copy link
Copy Markdown
Member Author

pjdarton commented Jul 28, 2017

@jswager Thank you for your endorsement on the Jenkins Developers group, https://groups.google.com/forum/#!topic/jenkinsci-dev/bvO_YOsYhR0 - I now have write access :-)

@elordahl I've closed 66 and merged 70-77 and 79.
I merged #70 because, by visual inspection, it looked reasonable.
I merged #79 because, by visual inspection, it's a much-needed bugfix.
I merged 71-77 as we'd agreed on that earlier.

I have not merged #78 because #80 may well solve the same problem that I was trying to solve with 78, but 80 does so with far less code. I'm in discussion with the author of that PR, @balleman , about this.

I've not merged #69 because it adds a dependency, it now has conflicts and, personally, I think that the existing logging facilities should be enhanced to the point where they provide understanding of when VM provisioning fails rather than using yet-another-system for this.
I am, however, open to persuasion on this, e.g. if other plugins already do this.

I've not merged #67 because I don't see these FindBugs warnings in either my IDE (I use Eclipse) or in the Jenkins build (findbugs doesn't seem to be being called) and I'm wary of making changes where I can't see the impact.
I would like us to have findbugs run over the code if we can, as code-analysis checking tools like this can find silly errors much more quickly than manual testing does, but it isn't something I know much about (I've never dealt with findbugs or telling maven to call it).
Also, the other changes made mean that this PR now has conflicts, so it's no longer a single button-push.

I've not merged #65 because the concerned raised earlier hadn't been addressed. Fortunately @phucyall has raised #79 and #81 that will achieve the same results, so I've closed 65 as all its remaining functionality is in 81.

FYI: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fvsphere-cloud-plugin/detail/master/9/ contains a build of the current master so we can download the interim build "2.16-SNAPSHOT (private-f3c7aace-jenkins)" from https://ci.jenkins.io/job/Plugins/job/vsphere-cloud-plugin/job/master/9/artifact/target/vsphere-cloud.hpi

@pjdarton
Copy link
Copy Markdown
Member Author

pjdarton commented Aug 4, 2017

@balleman @phucyall @elordahl @phoewass @MarcelMue We have a release-candidate - please test it :-)
https://ci.jenkins.io/job/Plugins/job/vsphere-cloud-plugin/job/master/13/artifact/target/vsphere-cloud.hpi
This is version "2.16-SNAPSHOT (private-17016511-win2012-67fc20$)"

We now have new functionality:

  • Bugfix: JENKINS-45786.
    Previously, setting an idle timeout period didn't work - there was a bug in the persistence code for the slave retention strategy.
    We should now be able to tell Jenkins to keep vSphere cloud slaves around until they've been idle for X minutes, and to keep on running builds on those VMs without having to spin up new ones all the time.
    (my work systems use Windows VMs a lot, which take ages to boot up compared to Linux ones, so I think this is awesome)
  • Bugfix: JENKINS-36952.
    It used to NPE if a VM's IP wasn't identified in time.
  • Bugfix: JENKINS-44796.
    We no longer leak VMs if vSphere can't be contacted at the time Jenkins decides to kill the slave.
    Previously, the plugin would have one attempt at removing slaves it didn't want anymore, and any exception would result in the slave staying around forever, which then prevented it from creating new slaves of that name.
  • Bugfix: Folder settings in Clone and Deploy build steps should now work via the webui.
  • Enhancement: timeoutInSeconds on the Clone and Deploy build steps.
    This should work for both pipeline usage and for jobs created via the job configuration page.
    It should seamlessly handle the upgrade scenario where old jobs are running with the new code.
  • Enhancement: Logging improvements.
    Tell Jenkins to log "vsphere-cloud" at loglevel ALL and the logging should be a little improved.
  • Updated yavijava to 6.0.05 (from 6.0.04).

Known weaknesses:

  • I can see in the code that not all build steps handle exceptions well - many still have "TODO throw AbortException" comments and just e.printStackTrace(). Clone has been fixed, but it should probably be handled in the parent class for all of them.
  • My bugfix for leaking VMs doesn't persist the list of "unwanted VMs" between Jenkins restarts. If vSphere was offline when it was trying to delete VMs and you reboot Jenkins, Jenkins will stop trying to delete them (but if it tries to re-create the VM and finds it already exists, it now reconnects). Ideally it should (re)discover existing slaves on startup.
  • Not even yavijava 6.0.05 has the option to abort a vSphere operation after a set timeout, so when vSphere decides that an operation will take an hour before declaring it a failure then there's not much we can do about it other than waiting an hour for the failure message. If anyone wants to tackle RFE: Add a maxTimeToWait parameter to Task.waitForTask yavijava/yavijava#238 then that'd be most appreciated.
  • The source code uses a mix of tabs and spaces. This has no effect on functionality but it does upset me :-) I'd welcome opinions on whether or not it's worth doing a wholesale "nothing uses tabs" or "nothing uses spaces" change. It would be simple to do, but it would create merge-hell for anyone who has forked the project.

What now:
I would like you all to give this new version a try and see if it works for you.
All the constituent parts have been tested, so it should all work, but nobody's tested it all together before. We need to test it and make sure there are no new problems before we release this as the next official release.
FYI I've installed this on my work Jenkins servers, we're using it "for real" and so we'll see if it works "for real" over the next few days.

  • If your tests indicate that it's OK, please comment here in this thread and say so.
  • If your tests indicate that it's broken...
    • if this is present in the 2.15 release, raise it on Jenkins JIRA (if it isn't logged already) and comment here in this thread.
    • if this fault is new in this version, either come up with a fix and raise a PR ... or comment here in this thread.

Note: I'm on vacation for a week (6th-13th August). I should have sporadic access to github/jira/email etc but not my dev/test environment (which is at work).

@pjdarton
Copy link
Copy Markdown
Member Author

@balleman @phucyall @phoewass I found another vSphere failure mode and edited the code to (hopefully) work around it.
New release-candidate is here: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fvsphere-cloud-plugin/detail/master/14/artifacts

Can you please let me know if this is working for you?

@pjdarton
Copy link
Copy Markdown
Member Author

pjdarton commented Sep 6, 2017

@elordahl Can you please log in to Artifactory as described by https://github.com/jenkins-infra/repository-permissions-updater#requesting-permissions

I suspect that this why jenkins-infra/repository-permissions-updater#376 failed to take effect and hence the reason behind INFRA-1327 ...
Once I've got permission to release, I'll push out 2.16 and then update the wiki page's screenshots etc.

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.

4 participants