Feat/add https support - #1017
Conversation
|
@rsandell is possible to have fast-forward merge for the patches. Allow to bisect faster any regression. This patch depends on last gerrit-events so will not compile |
I'm not sure what you mean by that? |
|
This is quite a big PR, would it be possible to divide it up a bit in chunks that can be merged separatrely? For example the new pipeline steps could be separate from the rest. |
|
Without looking at your implementation yet, could it be possible to integrate with https://plugins.jenkins.io/checks-api/ for the checks? or maybe that would be a separate plugin all together? |
Check api is already working, I have forked it. The problem was to support old check api here. I'm closing the circle to have complete ecosystem |
I mean preserve the history so we can bisect in the feature |
97539e0 to
7f73e8c
Compare
|
Will it support topic-based builds where a single build is triggered that includes multiple reviews? |
You mean mulpitle commits. Unless topic is not part of the information I get yes. I can give a try to it |
7f73e8c to
c98dd3a
Compare
|
@rsandell I need to check why it fails in jenkins CI |
8a0e0c8 to
a9fcf13
Compare
| public Object getTarget() { | ||
| Jenkins jenkins = Jenkins.get(); | ||
| // Allow unauthenticated access to webhook endpoint | ||
| String requestURI = Stapler.getCurrentRequest().getRequestURI(); |
There was a problem hiding this comment.
This should use Stapler.getCurrentRequest2
There was a problem hiding this comment.
That file should not be here I think. Accidently included in the commit?
| </div> | ||
| </f:section> | ||
| <script> | ||
| <![CDATA[ |
There was a problem hiding this comment.
Inline Javascript is a violation of stricter CSP rules that are available in since 2.541. This must be moved into a dedicated js file
| </j:choose> | ||
| </f:entry> | ||
| <!-- SSH-only fields: hidden/shown client-side when HTTPS polling toggled --> | ||
| <div class="ssh-only-section" style="${it.config.useHttpsPoller ? 'display:none' : ''}"> |
There was a problem hiding this comment.
Please use the class jenkins-hidden to hide something
| <div class="ssh-only-section" style="${it.config.useHttpsPoller ? 'display:none' : ''}"> | |
| <div class="ssh-only-section ${it.config.useHttpsPoller ? 'jenkins-hidden' : ''}"> |
In javascript then just toggle the class to show/hide it
| checked="${it.config.voteSameTopic}" | ||
| default="false"/> | ||
| </f:entry> | ||
| <div class="ssh-only-test-button" style="${it.config.useHttpsPoller ? 'display:none' : ''}"> |
a9fcf13 to
ed68685
Compare
|
@rsandell I will repush without webhook, they require more testing |
| version warning (orange settings icon) under HTTPS polling that does not | ||
| appear under SSH. This is cosmetic and does not affect functionality. | ||
|
|
||
| === Webhooks (New) |
There was a problem hiding this comment.
If you aren't including it in this PR perhaps this should be removed too?
| * SSH key configured for the Jenkins user in Gerrit | ||
| * `+Stream Events+` capability granted to the user | ||
|
|
||
| === HTTPS Polling (New) |
There was a problem hiding this comment.
| === HTTPS Polling (New) | |
| === HTTPS Polling |
|
|
||
| The plugin supports three transport mechanisms for receiving Gerrit events. | ||
| Choose the one that best fits your network environment and Gerrit setup. | ||
|
|
There was a problem hiding this comment.
There is technically a fourth option that has been in the plugin for a long time, I can't remeember the exact option, but it's "do not connect" it is to my knowlage only used together with the rabbitmq plugin somehow, where it adds tthe events to the queue.
There was a problem hiding this comment.
noConnectionOnStartup is the name of the variable controlling that "mode".
| See also the link:#connection-transport-options[Connection Transport Options] for information | ||
| about the new HTTPS polling and webhook transports. |
There was a problem hiding this comment.
Seems redundant?
| See also the link:#connection-transport-options[Connection Transport Options] for information | |
| about the new HTTPS polling and webhook transports. |
| private boolean restVerified; | ||
| private boolean useHttpsPoller; | ||
| private int httpsPollInterval; | ||
| private int httpsPollMaxChanges; |
There was a problem hiding this comment.
This confifguration class has been bloating up for a long time and I've been meaning to re-structure it for a long time, especially around the various connection modes. And now with one and a half more it is in dire need of it.
Itt is a bit tricky though of how to actually structure it.
There are various ways the trigger can receive events: ssh, http polling, and "no connection" for rabbitmq and web-hooks I guess.
Then there are at least two ways that it can deliver responses back: ssh commands and http-rest.
So it would be natural to divide the config up like that, but the two divisions uses configuration from eachother so it's not an even split like that.
If that work should be done, it should be done before we add even more fields to this class that would need to be readResolved when the restucture is made.
So any ideas are welcome. :)
There was a problem hiding this comment.
These new values could be grouped into one object and the check for if httppoll should be used it if that object is not null.
| } else if (!change.optString("number", "").isEmpty()) { | ||
| if (getServerConfig(serverName) != null) { | ||
| return getServerConfig(serverName).getGerritFrontEndUrlFor( | ||
| IGerritHudsonTriggerConfig config = getServerConfig(serverName); |
There was a problem hiding this comment.
this should be extracted into something like getServerConfigOrFirst(String serverName)
| serverName = name; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
getServerConfigOrFirst(String serverName) might be reusable here?
| /* | ||
| * The MIT License | ||
| * | ||
| * Copyright 2024 Sony Mobile Communications Inc. All rights reserved. |
There was a problem hiding this comment.
Weird choice of year, also wrong attribution.
| /* | ||
| * The MIT License | ||
| * | ||
| * Copyright 2024 Sony Mobile Communications Inc. All rights reserved. |
There was a problem hiding this comment.
@rsandell I will repush tonight with all the fixes, and I need to test it again
| Jenkins jenkins = Jenkins.get(); | ||
| // Allow unauthenticated access to webhook endpoint | ||
| String requestURI = Stapler.getCurrentRequest2().getRequestURI(); | ||
| if (requestURI != null && requestURI.endsWith("/webhook")) { |
There was a problem hiding this comment.
I don't know but you might be circumventing more than intended. For example a post to gerrit-trigger/webhook/configSubmit or a get gerrit-trigger/webhook/serverNames
There was a problem hiding this comment.
Perhaps it would be safer/easier to just have the webhook as a separate root action. For example jenkins/gerrit-trigger-webhook/.
| */ | ||
| public static String makeGerritGitUrl(String frontEndUrl, String project) { | ||
| StringBuilder url = new StringBuilder(frontEndUrl); | ||
| if (!frontEndUrl.endsWith("/")) { |
There was a problem hiding this comment.
hudson.Functions#joinPath does this for you.
Add a computed environment parameter GERRIT_GIT_URL that provides the
git-over-HTTPS clone URL for the Gerrit project being built.
Format: {frontend_url}/a/{project}.git
This enables users to configure their git SCM step in Jenkins pipelines
to use HTTPS instead of SSH:
Repository URL:
Branch Specifier:
Changes:
- Add GERRIT_GIT_URL enum to GerritTriggerParameters
- Add StringUtil.makeGerritGitUrl(frontEndUrl, project) utility
- Add getGitURL() helper method (models the existing getURL() pattern)
- Set the parameter value during ChangeBasedEvent parameter creation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Replace server-side <j:if> conditions with CSS classes and JavaScript so that toggling 'Use HTTPS Polling for Events' immediately shows/hides SSH-specific fields without requiring a page save and reload. - Wrap SSH fields in <div class='ssh-only-section'> with initial state set via inline style based on the saved config value - Wrap Test Connection button in <div class='ssh-only-test-button'> - Add CDATA-wrapped JavaScript that listens on the useHttpsPoller checkbox change event and toggles display:none on both sections - Fields that toggle: SSH Port, Proxy, Username, SSH Keyfile, SSH Keyfile Password, Test Connection button Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
The Gerrit REST API /a/config/server/version returns the full git-describe version (e.g. '3.13.3-877-g4b89419c9d'), while the SSH 'gerrit version' command returns only the base release tag (e.g. '3.13.3'). If the Gerrit server runs a development/snapshot build, the plugin may show a snapshot version warning (orange settings icon) under HTTPS polling that does not appear under SSH. Document this behavior in the help text so users know it's cosmetic and does not affect functionality. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
… serverName guard - GerritServer.setConfig(): set gerritQueryHnadler to null instead of eagerly creating it, so getQueryHandler() can lazily create a fresh handler with the correct type (GerritRestQueryHandler vs GerritQueryHandler) and fully-configured authentication. - ManualTriggerAction.getGerritUrl(): add fallback to first enabled server when serverName is null or config not found, matching the established pattern in getFrontEndUrl(). - ManualTriggerActionTest: pass concrete server name to match the mock setup, since getServerConfig(null) now correctly returns null after the null guard added in dad9686. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
The new implementation requires GerritRestPoller, GerritRestQueryHandler, and GerritEventSource which only exist in 2.23.0. Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Decompose the monolithic Config.java by extracting 12 vote value fields into GerritVoteValues and 6 command template fields into GerritCommandTemplates. Config becomes a facade that delegates to these sub-objects while maintaining full XStream serialization backward compatibility. - GerritVoteValues: holds verified+code-review vote values for all 6 build outcomes with defaults, JSON parsing, copy, and readResolve() - GerritCommandTemplates: holds the 6 gerrit review command templates with defaults, JSON parsing, copy, and readResolve() - Old flat fields marked transient in Config; readResolve() migrates them into the new sub-objects on deserialization - IGerritHudsonTriggerConfig interface unchanged — all callers work without modification - JCasC binding intact via delegation setters on Config Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
ed68685 to
7fe43ae
Compare
@rsandell yes I have added a token, to webhook, I will post after this series get merged. The problem is on gerrit itself that seems does not support HMAC. |
|
@panicking please stop force pushing, it makes it very hard to see what I have already reviewed and I need to restart the review from the beginning every time you push fixes. |
| public Config(IGerritHudsonTriggerConfig config) { | ||
| gerritHostName = config.getGerritHostName(); | ||
| gerritSshPort = config.getGerritSshPort(); | ||
| gerritProxy = config.getGerritProxy(); |
There was a problem hiding this comment.
specific ssh related and http related config could maybe be grouped into classes as well?
| @@ -896,7 +845,7 @@ | |||
|
|
|||
| @Override | |||
| public String getGerritCmdBuildSuccessful() { | |||
There was a problem hiding this comment.
| public String getGerritCmdBuildSuccessful() { | |
| @Deprecated | |
| public String getGerritCmdBuildSuccessful() { |
with javadoc pointing to the replacement.
| * | ||
| * @return the vote values sub-object, never null. | ||
| */ | ||
| private GerritVoteValues getVoteValuesInternal() { |
There was a problem hiding this comment.
| private GerritVoteValues getVoteValuesInternal() { | |
| private synchronized GerritVoteValues getVoteValuesInternal() { |
could also be
| private GerritVoteValues getVoteValuesInternal() { | |
| public synchronized GerritVoteValues getVoteValues() { |
so that the old deprecated methods can point to something? But I guess that would require changes to the interface as well which is probably ok now that we can do default methods :)
| * | ||
| * @return the command templates sub-object, never null. | ||
| */ | ||
| private GerritCommandTemplates getCommandTemplatesInternal() { |
There was a problem hiding this comment.
needs to be synchronized and perhaps public as well?
| // Migrate old flat vote-value fields into the sub-object | ||
| if (this.voteValues == null) { | ||
| this.voteValues = new GerritVoteValues(); | ||
| if (this.gerritBuildStartedVerifiedValue != null) { |
There was a problem hiding this comment.
good catch! I guess this is to guard against first time configuration vs migration of old/changed values?
| /* | ||
| * The MIT License | ||
| * | ||
| * Copyright 2026 Amarula Solutions. All rights reserved. |
There was a problem hiding this comment.
most of the code in this class looks copied, so I think you need to keep the original attribution as well as adding your own.
These things are tricky :)
| * | ||
| * @param formData the JSON object with form data. | ||
| */ | ||
| public void setValues(JSONObject formData) { |
There was a problem hiding this comment.
If the form is correctly structured this should not be needed any more and using @DataboundSetter and @DataboundContructor can be used instead of this antequated manual json stuff :).
| * | ||
| * @return the resolved instance. | ||
| */ | ||
| Object readResolve() { |
There was a problem hiding this comment.
I don't think this is needed, old versions didn't have this class at all so there is nothing to resolve from.
There was a problem hiding this comment.
@rsandell we need to agree on the approach
Sorry, I'm working with gerrit and usually I don't like to create fix on top of pull request, so I usually rebase the changes and fix your comment on each commit where this code was added. What you suggest? I mean I can create fixes and the rebase and squash on commits. Is that ok? |
|
Yea, I know, It also felt very weird for me comming from Gerrit's way of handling it to GitHub's PR model :) |
GerritVoteValues is a new class introduced in this PR, so no old serialized data exists for XStream to resolve. The backward compatibility for aborted vote values is already handled in Config.readResolve() before values are migrated into the sub-object. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
GerritVoteValues and GerritCommandTemplates were extracted from Config.java. The original Config.java copyright (2010 Sony Mobile Communications Inc.) must be retained alongside the new attribution since most of the code was copied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
…lues/getCommandTemplates Make these accessors public synchronized so callers can use the sub-objects directly instead of going through deprecated Config methods. The old Internal() suffixes suggested private usage only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Add @deprecated to getGerritCmdBuildSuccessful(), getGerritCmdBuildUnstable(), getGerritCmdBuildFailed(), getGerritCmdBuildStarted(), getGerritCmdBuildNotBuilt(), and getGerritCmdBuildAborted(). Each includes javadoc pointing to the replacement on GerritCommandTemplates accessed via getCommandTemplates(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Add @deprecated to all 6 setGerritCmdBuild* methods and update javadoc on all 12 command setters (setGerritVerifiedCmdBuild* and setGerritCmdBuild*) to point to GerritCommandTemplates.setCmdBuild*() replacements. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Add getServerConfigOrFirst(serverName) to ManualTriggerAction to eliminate duplicate fallback-to-first-server pattern used in both getFrontEndUrl() and getGerritUrl(). Add getServerOrFirst_(name) to PluginImpl to eliminate the same pattern in GerritTriggerParameters.getGitURL(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
…jectsViaRest Replaces StringBuilder-based URL construction with the standard hudson.Functions.joinPath utility method. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
ed55850 to
1e19e8a
Compare
|
@rsandell Is ok this way? I need to retest again the plugin now, but working on incrementatl changes |
|
yes it's ok to continue! Thanks!. And sorry again for the late reply. |
Pull Request: Gerrit Pipeline Steps, HTTPS Polling, and Webhook Support
Needs sonyxperiadev/gerrit-events#128
Overview
This pull request introduces significant feature enhancements to the Jenkins Gerrit Trigger plugin. It adds three new pipeline steps for deeper Gerrit integration (checks and reviews), implements an alternative HTTPS polling mechanism (reducing reliance on SSH), and introduces a new webhook endpoint for receiving Gerrit events directly over HTTP.
Key Features
1. HTTPS Polling and REST Integration
To remove the strict dependency on SSH for event polling and querying, HTTPS/REST alternatives have been fully integrated:
useHttpsPoller, interval settings, and max changes configuration to the server UI.GerritRestPollerandGerritRestQueryHandlerto route queries and polling through the Gerrit REST API instead of SSH commands.GET /a/projects/?dwhen HTTPS polling is enabled.GerritServerto use the generalizedGerritEventSourceinterface.2. Webhook Support
POST /gerrit-trigger/webhook?server={name}that acceptsstream-eventsJSON payloads from Gerrit's webhook or events-log plugin.GerritWebhookCrumbExclusionsince webhook callers cannot provide CSRF tokens.3. UI and Quality-of-Life Improvements
GERRIT_GIT_URLParameter: Injects a computed HTTPS git clone URL (format:{frontend_url}/a/{project}.git) for pipeline scripts and freestyle jobs.4. Documentation and Tests
GERRIT_GIT_URL.Authors
Testing done
Deploy on real infrastructure, before this rebase. Tested almost all the interface except webhook and gerrit check post. Require the gerrit-events
Submitter checklist