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

fixes to [wercker] badge #2103

Merged
merged 5 commits into from
Sep 27, 2018
Merged

fixes to [wercker] badge #2103

merged 5 commits into from
Sep 27, 2018

Conversation

chris48s
Copy link
Member

Started looking at #2076 and as I dug into it found some more issues. I've made several changes to the wercker integration in this PR:

  1. If a application or branch does exist but doesn't have any builds or CI runs, a response of [] is returned. Example: https://app.wercker.com/api/v3/applications/wercker/stern/builds I've modified the validation and added a case for this.

  2. Our current/previous regex was not working correctly when using a project ID:

    If I call https://img.shields.io/wercker/ci/559e33c8e982fc615500b357.svg that generates based on (correctly) calling https://app.wercker.com/getbuilds/559e33c8e982fc615500b357?limit=1

    If I call https://img.shields.io/wercker/ci/559e33c8e982fc615500b357/master.svg that generates because "559e33c8e982fc615500b357/master" is matching the regex pattern ([^/]+/[^/]+) so we are (incorrectly) calling https://app.wercker.com/api/v3/applications/559e33c8e982fc615500b357/master/builds?limit=1 instead of (correctly) calling https://app.wercker.com/getbuilds/559e33c8e982fc615500b357?limit=1&branch=master .

    I've updated the regex so we try to specifically match ([a-fA-F0-9]{24}) first then fall back to ([^/]+/[^/]+) which fixes that (unless, of course, your github username is fb1ed340ffed75c22dc301c3, in which case I broke it 😄 )

  3. Following on from Is Wercker badge reporting the correct status? #2076 I've changed this so that:

    • ci/{projectId} reports the result of a CI run (totally sensible)
    • build/{projectId} reports the result of a build (totally sensible)
    • build/{applicationName} reports the result of a build (totally sensible)
    • ci/{applicationName} reports the result of a build but isn't documented anywhere (I'm still in 2 minds about whether this is actually the right thing to do here)

I've also updated/added tests accordingly.

Annoyingly, this also has the side effect that it probably makes this a bad example to base documentation on, so I probably now want to go back and change #2075 but lets get this right first and then I'll worry about what I'm going to do with that.

- Fix case where app/branch exists but has no builds or CI runs
- Fix aplicationId/branch regex case
- Issue badges#2076:
  - `ci/{projectId}` reports the result of a CI run
  - `build/{projectId}` reports the result of a build
  - `build/{applicationName}` reports the result of a build
  - `ci/{applicationName}` reports the result of a _build_
    (but isn't documented anywhere) for "backwards compatibility"
@shields-ci
Copy link

Messages
📖

✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS

@PyvesB
Copy link
Member

PyvesB commented Sep 21, 2018

As far as 2. is concerned, when I added branch support in #1920 I actually didn't intend on supporting branches for our old ID based URL, as it had no longer been advertised on the homepage for years. I simply carried over the backwards compatibility for old badges that may still be lying around, so I wouldn't actually say that it "was not working correctly". 😄

This PR looks overall good to me, but I don't think we should add the new build/{projectId} URL. It maps to https://app.wercker.com/getbuilds underneath the hood, which I believe is the old undocumented and deprecated API, which we started phasing out on our side in #456. It seems wrong to add new Shields badge based on it.

In my opinion we should only have build/{applicationName} and ci/{projectId}, and keep ci/{applicationName} for backwards compatibility but not actually advertise it on the homepage.

Nevertheless, it would be nice for users to use ci/{applicationName} without having to figure out their project ID, maybe we raise a ticket with Wercker so that support using names for the /runs endpoint? Our badges would then be consistent with build/{applicationName} and ci/{applicationName} as main URLs, and we could hide ci/{projectId} again and only keep it for backwards compatibility.

Does this make sense?

@chris48s
Copy link
Member Author

..so I wouldn't actually say that it "was not working correctly"

OK, well since we can make it work, we might as well while I'm at it..

In my opinion we should only have build/{applicationName} and ci/{projectId}, and keep ci/{applicationName}

Done

@chris48s chris48s merged commit 4f47cb3 into badges:master Sep 27, 2018
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants