refactor [JenkinsBuild] #3302
refactor [JenkinsBuild] #3302
Conversation
calebcartwright
changed the title
|
calebcartwright
changed the title
|
Haven't given this a full review, though was playing around with #1956 (comment) and I noticed a disparity and thought I'd mention it. This on master: is equivalent to on this branch. |
|
Fair point on the paths. It looks to me as if the legacy service did its best to support users whether they simply mentioned a job name There is an issue here with the route path though, in this PR it is currently: I believe the consecutive One way I can fix this would be to revert the pattern back to: However, this does require the user to enter the host path info, and the job prefix, and the job name in the route This would not work (since the host is on the and would require: |
calebcartwright
changed the title
calebcartwright
changed the title
|
I've updated the route to mirror the existing/current route |
|
| errorMessages = { 404: 'instance or job not found' }, | ||
| disableStrictSSL, | ||
| }) { | ||
| const options = { qs, strictSSL: disableStrictSSL === undefined } |
There was a problem hiding this comment.
See below comment in jenkins-common.js for more info on param name
| label: 'build', | ||
| } | ||
| } | ||
|
|
||
| static get route() { | ||
| return { | ||
| base: 'jenkins/s', |
There was a problem hiding this comment.
I don't have any PR #'s handy, but we seem to be trending away from single letter aliases to more explicit route param names.
I'd like to do the same thing here (build or status instead of s). Any thoughts/objections?
There was a problem hiding this comment.
Happy if you want to change it, but we'll want to maintain an alias/redirect for /s for existing users
We talked about formalising some of this stuff in docs in #3144 (comment) and then.. haven't got round to doing it yet. There's several things in that discussion that I want to get done at some point but haven't found the time. Having this stuff documented would make it easier when we're thinking
wait, what is our convention for a "build status" URL?
🙂
There was a problem hiding this comment.
I'll leave it in place for now. I plan on using some of the common bits in here to refactor the Jenkins test service so I'll consider the alias redirecting in that one
| const Joi = require('joi') | ||
|
|
||
| const queryParamSchema = Joi.object({ | ||
| disableStrictSSL: Joi.equal(''), |
There was a problem hiding this comment.
Although I usually try to refrain from using negated variable names, naming this with the disable prefix seemed to be the best approach given the default behavior we want here and the way boolean query params are handled (empty string when set, undefined when not set).
With this approach, a user will have to explicitly include the disableStrictSSL query param:
https://shields-staging-pr-3302.herokuapp.com/jenkins/s/https/jenkins-oss.wixpress.com/job/detox-android-56-master.svg?disableStrictSSL
As the default behavior (no query param provided) will still enforce strict ssl, which is why the below badge will fail (there's a cert chain issue with the target jenkins instance)
https://shields-staging-pr-3302.herokuapp.com/jenkins/s/https/jenkins-oss.wixpress.com/job/detox-android-56-master.svg
Definitely open to feedback/suggestions/etc. if anyone has any alternative ideas!
| return `${protocol}://${host}/${jobPrefix}${job}/${ | ||
| lastBuild ? 'lastBuild/' : '' | ||
| }${plugin ? `${plugin}/` : ''}api/json` | ||
| } |
There was a problem hiding this comment.
I'm setting this function up to be re-used in all the other Jenkins services (Tests, etc.) which is why it contains things not explicitly used for the Build service badge
| namedParams: { | ||
| scheme: 'https', | ||
| protocol: 'https', | ||
| host: 'jenkins.qa.ubuntu.com', |
There was a problem hiding this comment.
I am also intentionally not including the new disableStrictSSL query param here as I do not believe this is something we want to advertise; it's a work around for a handful of exceptional circumstances only.
Refs #2863