-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
The Great [Wercker] Rewrite #1920
Conversation
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - looks good. Left one minor comment.
services/wercker/wercker.tester.js
Outdated
|
||
t.create('CI connection error)') | ||
.get('/ci/wercker/go-wercker-ap.json') | ||
.networkOff() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this is check handled in the base class, we don't need to explicitly test it for every service any more unless there is some special handling of this case.
It is still useful for the services defined in server.js
where we're doing the checks manually, but as we port stuff over to the new layout we can dispense with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point - removed that test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for taking this on!
services/wercker/wercker.service.js
Outdated
: `https://app.wercker.com/getbuilds/${projectId}?limit=1` | ||
if (branch) { | ||
url += `&branch=${branch}` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better handled, IMO, by including options: { query: { branch} }
in the call to requestJson()
, which I believe will do the right thing. Plus it will URL-encode it, if necessary, and avoids any possibility of a vulnerability where extra parameters could be injected into our request via branch
. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. After some trial and error, I believe qs
is actually the right field to set here.
static render({ status, result }) { | ||
if (status === 'finished') { | ||
if (result === 'passed') { | ||
return { message: 'passing', color: 'brightgreen' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are translating passed to passing, why does the build status regex need to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I must have added this part way through rewriting the service, didn't think about removing it later.
services/wercker/wercker.service.js
Outdated
async handle({ applicationName, projectId, branch }) { | ||
const json = await this.fetch({ applicationName, projectId, branch }) | ||
const status = json[0].status | ||
const result = json[0].result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can assign these using destructuring:
const { status, result } = json[0]
|
||
t.create('CI private application') | ||
.get('/ci/wercker/blueprint.json') | ||
.expectJSON({ name: 'build', value: 'private application not supported' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy about how this tied into my previous piece of work, #1888, which made things really easy here. 😄
Thanks for reviewing @chris48s and @paulmelnikow! |
Users seem to have been dissatisfied with our Wercker badge offering for quite some time (see #825, #604, #980 and #455). This pull request attempts to improve things:
Looking forward to feedback! 👍