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

Pipeline status badge endpoints for server #158

Merged
merged 8 commits into from Feb 3, 2019
Merged

Pipeline status badge endpoints for server #158

merged 8 commits into from Feb 3, 2019

Conversation

brew
Copy link
Contributor

@brew brew commented Jan 31, 2019

The dashboard server included a badge endpoint that was unfortunately broken. This PR fixes up the existing badge endpoint to use the updated status object, and introduces a new /badge/collection endpoint to return a summary status of a pipeline collection. Explanation added to the README to illustrate their use:


Even simpler pipeline status is available with a status badge, both for individual pipelines, and for pipeline collections. For a single pipeline, add the full pipeline id to the badge endpoint:

http://localhost:5000/badge/path_to/pipelines/my-pipeline-id

Or for a collection of pipelines:

http://localhost:5000/badge/collection/path_to/pipelines/

Note that these badge endpoints will always be exposed regardless of DPP_BASIC_AUTH_PASSWORD and DPP_BASIC_AUTH_USERNAME settings.


Note, this PR also removes a refresh endpoint, that I don't think is actively in use, but looked like an unnecessary DoS target.

Some previous status manager refactoring broke the badge end point. I've
also changed it from redirecting to the shields.io image, to proxying
it.
If no pipeline_id is found, return a badge with 'not found' written in
it, rather than returning a 404. Knowing that a pipeline is potentially
missing is still useful status.
A status badge at the /badge/collection/<pipeline path> endpoint.

This returns a pipeline collection summary badge, e.g.

pipelines | 22 succeeded, 14 failed, 2 invalid, 1 running

Pipeline path will filter the pipeline ids it is reporting on.
This commit makes application of basic auth (if active) more selective.
Instead of forcing basic auth on all endpoints, it is now applied as a
decorator for the main dashboard views. This allows us to serve the
status badges without basic auth while still being able to protect the
dashboard pages.
@brew brew requested a review from akariv January 31, 2019 14:22
@brew brew changed the title Pipeline status badges endpoints for server Pipeline status badge endpoints for server Jan 31, 2019
@coveralls
Copy link

Pull Request Test Coverage Report for Build #88

  • 0 of 66 (0.0%) changed or added relevant lines in 1 file are covered.
  • 214 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-9.1%) to 17.039%

Changes Missing Coverage Covered Lines Changed/Added Lines %
datapackage_pipelines/web/server.py 0 66 0.0%
Files with Coverage Reduction New Missed Lines %
datapackage_pipelines/web/server.py 4 0.0%
.tox/py36-sqlite/lib/python3.6/site-packages/datapackage_pipelines/web/server.py 105 0.0%
.tox/py36-plyvel/lib/python3.6/site-packages/datapackage_pipelines/web/server.py 105 0.0%
Totals Coverage Status
Change from base Build 1009: -9.1%
Covered Lines: 1695
Relevant Lines: 9948

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build #88

  • 0 of 66 (0.0%) changed or added relevant lines in 1 file are covered.
  • 214 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-9.1%) to 17.039%

Changes Missing Coverage Covered Lines Changed/Added Lines %
datapackage_pipelines/web/server.py 0 66 0.0%
Files with Coverage Reduction New Missed Lines %
datapackage_pipelines/web/server.py 4 0.0%
.tox/py36-sqlite/lib/python3.6/site-packages/datapackage_pipelines/web/server.py 105 0.0%
.tox/py36-plyvel/lib/python3.6/site-packages/datapackage_pipelines/web/server.py 105 0.0%
Totals Coverage Status
Change from base Build 1009: -9.1%
Covered Lines: 1695
Relevant Lines: 9948

💛 - Coveralls

Copy link
Member

@akariv akariv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall approved, only one question/comment.

@@ -220,27 +240,76 @@ def pipeline_api(field, pipeline_id):
return jsonify(ret)


def _make_badge_response(subject, text, colour):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering why not do a redirect here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it redirects away from the site, you can't change the url as easily, to see different results.

@akariv akariv merged commit 3e11074 into master Feb 3, 2019
@akariv akariv deleted the server-status branch February 3, 2019 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants