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-40059] CommentAdded trigger firing on every comment #302

Merged
merged 1 commit into from Apr 11, 2017

Conversation

Projects
None yet
3 participants
@tyroneabdy
Copy link
Contributor

commented Nov 28, 2016

The CommentAdded trigger was firing on any comment after
the necessary value of an approval had been met.
This was happening due to that in previous versions of gerrit
before 2.13.0 approval information only appeared once
something had changed.
This changed to use the oldValue that would optionally
be included on state change. The logic added
to the gerrit-trigger-plugin was based on an earlier
patchset that had an ever present variable for state
change.
This led to a fall through to the old style of checking
for whether to trigger based on a comment.

return true;
GerritServer server = PluginImpl.getServer_(serverName);
if (server.getGerritVersion() != null
&& server.getGerritVersion().compareTo("2.13.0") >= 0) {

This comment has been minimized.

[JENKINS-40059] CommentAdded trigger firing on every comment
The CommentAdded trigger was firing on any comment after
the necessary value of an approval had been met.
This was happening due to that in previous versions of gerrit
before 2.13.0 approval information only appeared once
something had changed.
This changed to use the oldValue that would optionally
be included on state change. The logic added
to the gerrit-trigger-plugin was based on an earlier
patchset that had an ever present variable for state
change.
This led to a fall through to the old style of checking
for whether to trigger based on a comment.

@tyroneabdy tyroneabdy force-pushed the tyroneabdy:master branch from 85d4c92 to 0a8e1c2 Nov 29, 2016

@scoheb

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

Hi @rsandell !

Any chance we can get this released? Is there anything else to address?

@rsandell rsandell closed this Apr 10, 2017

@rsandell rsandell reopened this Apr 10, 2017

@rsandell

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Just tickled the pr builder.

@rsandell
Copy link
Member

left a comment

Some of the test failures seems legit.

@scoheb

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

Test failures fixed here: #309

@rsandell rsandell merged commit a36413b into jenkinsci:master Apr 11, 2017

1 check failed

Jenkins
Details
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.