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-38675] use refspec instead of branch name for the revision #299

Merged

Conversation

Projects
None yet
2 participants
@jacob-keller
Copy link
Contributor

commented Oct 5, 2016

In the GerritTriggerBuildChooser, we have a custom method for
determining which revision to build. However, this revision is not
actually a branch but is instead a Gerrit refspec of the built event.
Instead of reporting the branch which is almost certainly meaningless to
the Git summary in our case, report the refspec string instead. We'll
use the branch as a fallback when running a build which was not built
due to a Gerrit event.

The result of this change is that we should see something like:

Revision:
refs/changes/x/xxxx/x

Instead of

Revision:
master

This produces a much more useful summary since it does not help to tell
the user "we built master" when instead we actually built one of the
Gerrit revisions that has not yet been committed.

Signed-off-by: Jacob Keller jacob.e.keller@intel.com

@rsandell
Copy link
Member

left a comment

Hmm, I was expecting at least one unit test to fail, but since none did I guess we have poor test coverage in this area. So It would be much simpler to merge with confidence if a test or two where added (that works before this change and after with just a slight change of the string to expect.
You could probably use GitSampleRepoRule from the git plugin to easely construct a valid git repo for the test. You can see sample usage of it in the pipeline suite of plugins and pipeline-model-definition-plugin.

@jacob-keller

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2016

Yea, I agree. I have been thinking about how exactly to test this, thanks for the suggestions.

Jacob Keller
[JENKINS-38675] use refspec instead of branch name for the revision
Add support to the GerritTriggerBuildChooser enabling it to report the
gerrit change refspec instead of simply reporting the branch this change
is destined to. This improves readability of the resulting changelog
entry on the Jenkins build page, as the user can see exactly which ref
was built.

To do so, we need to not only ask for the revision, but the refname. Add
a context callback in order to determine in a similar way as we do the
revision. Although it might be possible to replace the revision handling
to use git.revParse() on the refname instead, this may not be as
accurate as the change event, so we leave this in place.

The result of this change is that we should see something like:

Revision: <sha1id>
  refs/changes/x/xxxx/x

Instead of

Revision: <sha1id>
  master

The resulting display is easier to understand and more accurately
points out the refname that got built.

Add a test to ensure that this behaves as expected. The current test can
probably be improved as it relies heavily on mock objects and does not
actually generate a trigger event or anything of that sort.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

@jacob-keller jacob-keller force-pushed the jacob-keller:jk/gerrit-revision-show-refspec branch from 0bd2d49 to 89eca5e Oct 14, 2016

@jacob-keller

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2016

@rsandell I added a test. I don't think it's idea but I didn't manage to go down the GitSampleRepoRule path just yet. The current test relies heavily on mocking objects. Let me know if you have better suggestions.

I also found a bug in the previous revision as I had confused revisions and refname. The new version has a separate context for grabbing the gerrit event. It might be worth refactoring that more to avoid using context twice? Can we get context to return the actual gerritEvent and then let the main section determine what to do here? I wasn't sure how that worked.

@jacob-keller

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2016

It should be noted that GitSampleRepoRule is not in any released version of the git plugin, except for beta 3.0.0-beta1, so I don't think we can depend on it for the test easily :(

I think the mocked test is ok enough at the moment but it does seem pretty fragile.

@jacob-keller

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2016

I added a second follow on patch which shows my suggested alternative above. I do not think I know which I prefer at this point, and I really don't like the way the test is implemented with a copied BuildChooserContextImpl but I can't find a better way.

@jacob-keller jacob-keller force-pushed the jacob-keller:jk/gerrit-revision-show-refspec branch from 1595772 to 89eca5e Oct 14, 2016

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

Well I was hoping for a more integration type test, using JenkinsRule that triggered a build with the GerritTriggerBuildChooser set and verified that the correct rev got checkedout in the workspace and that the changelog showed correctly. But this is at least a lot better than nothing :)

@rsandell rsandell merged commit 670fbe2 into jenkinsci:master Oct 17, 2016

1 check passed

Jenkins This pull request looks good
Details
@jacob-keller

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2016

I couldn't figure out how to get that to work. We can always add an
additional test later though.

On Oct 17, 2016 3:43 AM, "Robert Sandell" notifications@github.com wrote:

Well I was hoping for a more integration type test, using JenkinsRule
that triggered a build with the GerritTriggerBuildChooser set and
verified that the correct rev got checkedout in the workspace and that the
changelog showed correctly. But this is at least a lot better than nothing
:)


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#299 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA1Y3ypoSxvUdE0SzBh14U-gtrQpp1H0ks5q01FBgaJpZM4KPF19
.

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.