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

Fix checkout by version spec #36

Merged
merged 23 commits into from
Apr 9, 2015
Merged

Conversation

olivierdagenais
Copy link
Member

Two defects were discovered in the implementation of pull request #24:

  1. The value of VERSION_SPEC was incorrectly applied.
  2. The reported SCM changes between builds by version spec was incorrectly querying TFVC.

Outstanding weirdness/defect:

I created a job with polling, then added the VERSION_SPEC parameter with a default value of LFoo (a label that pointed to changeset 4). Running a build indeed got the source from changeset 4, but then subsequent polling would detect that a changeset newer than 4 existed and queue a build. The build would again obtain changeset 4 and the loop repeated itself until I turned off polling or removed the default value.

Part of the problem was due to configuration error (it probably doesn't make sense to supply a default value to a VERSION_SPEC parameter - I only did it because I'm testing the feature), but a configuration error should NOT cause a build loop. The loop itself is difficult to cause for most users, since the "checkout by label" feature isn't [yet] documented nor easily discoverable, so it might not be a very big deal.

@olivierdagenais
Copy link
Member Author

@TomaszDom and @kopcheski, do you want to weigh in on the subject? @mosabua has already gone through this pull request's commits once, so I would appreciate hearing from users who are using (or want to use) the "checkout by label" feature. Caveat emptor: if you want to test the plugin built by this branch, be aware that while an upgrade will work fine, a downgrade (back to the previously stable version of the TFS plugin) will result in having to re-enter the TFS user's password for all jobs.

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@mosabua
Copy link
Member

mosabua commented Apr 1, 2015

+1 for merging this. I will do some further testing on this today, but initial usage seems everything is fine.

@olivierdagenais
Copy link
Member Author

I will do some further testing on this today, but initial usage seems everything is fine.

@mosabua: Can you report on your soak test?

Since we expected VERSION_SPEC to be prefixed with "L" for labels, it
wasn't a very big stretch to adapt and rename the methods to go by a
single version spec (as opposed to a range thereof).
This will make the fact that a DateVersionSpec is special for the
command completely transparent, instead of having separate overloads.
getVersionSpecificationWhenLabelVersionSpecWithoutScope() exposed a
defect, which was worked around (hopefully temporarily) by adding
another special case.
@olivierdagenais olivierdagenais changed the title Draft: Fix checkout by version spec Fix checkout by version spec Apr 8, 2015
@mosabua
Copy link
Member

mosabua commented Apr 8, 2015

I got pulled away with other issues after local smoke testing. That went fine. I am hoping to do some further testing on the staging server next week. I would suggest to merge away thoug .. and then fix prior to a release if we find anything.

olivierdagenais added a commit that referenced this pull request Apr 9, 2015
@olivierdagenais olivierdagenais merged commit 6e6d90e into master Apr 9, 2015
@mosabua
Copy link
Member

mosabua commented Apr 9, 2015

Nice!

@mosabua
Copy link
Member

mosabua commented Apr 13, 2015

Should delete the branch probably..

@olivierdagenais olivierdagenais deleted the fixCheckoutByVersionSpec branch April 14, 2015 00:14
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.

3 participants