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

#158212394 Build health check url #42

Merged
merged 54 commits into from
Jul 2, 2018
Merged

Conversation

dkam26
Copy link
Contributor

@dkam26 dkam26 commented Jun 14, 2018

What does this PR do?

Have a healthcheck Url to confirm db is up and running

Description of Task to be completed?

Have the endpoint below working:

GET /_healthcheck

How should this be manually tested?

After setting up the project on your locahost.

Using a web browser,test the URL.

What are the relevant pivotal tracker stories?

#158212394

Screenshots

When the database up and running.

image

When the database doesn't exist or down.

image

@coveralls
Copy link

coveralls commented Jun 14, 2018

Pull Request Test Coverage Report for Build 399

  • 15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 99.167%

Totals Coverage Status
Change from base Build 393: 0.04%
Covered Lines: 357
Relevant Lines: 360

💛 - Coveralls

Readme.md Outdated
@@ -58,7 +58,7 @@
```
- Run application.
```
python manage.py runserver

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is manage command for starting server removed from the readme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PatrickCmd ,Thank you.It was a typo.Let me fix it

schema=healthcheck_schema,
graphiql=True # for healthchecks
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the health check url would be a template for visualization if the services are okay but not an endpoint. What I mean by this, is having a route for a view function that renders this template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PatrickCmd ,the story description was changed to an endpoint.

return res


healthcheck_schema = graphene.Schema(query=Query)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think what should be in this module must look like what is in the main.py module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PatrickCmd ,I dont get what you saying.Please rephrase your feedback.

requirements.txt Outdated
coveralls
urllib3==1.23
virtualenv==15.2.0
Werkzeug==0.14.1
Copy link
Contributor

Choose a reason for hiding this comment

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

@dkam26 Your requirements file is not following the best practice we decided as the backend team. Please take a review at the one on develop.

@fidelisojeah
Copy link
Contributor

@dkam26 for this health check

  • Can you show the what the expected output is supposed to be if successful and if unsuccessful (in your PR request)

@@ -23,6 +24,14 @@ def create_app(config_name):
graphiql=True # for having the GraphiQL interface
)
)
app.add_url_rule(
'/_healthcheck',
view_func=GraphQLView.as_view(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this healthcheck has to be a graphQL element
a regular endpoint with a JSON kind of response is fine (Though the graphQL one is also okay).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fidelisojeah ,All is done

Walukagga Patrick and others added 13 commits June 14, 2018 17:50
* Add untracked  files after merge

* [Fix] Pull Updated changes to develop

* [Feature #158429512] Add Delete room functionality

 This PR adds the delete room functionality where a user can delete a room once they have the correct roomId.This commit also containes tests for this feature.
* [Bug 158140748] Restrict deletion of room resources to admin

[Bug 158140748] Add location for backward relationship

[Bug 158140748] Add block for backward relationship

[Bug 158140748] Add floor for backward relationship

[Bug 158140748] Add room for backward relationship

[Bug 158140748] Add decorator function to restrict mutation for admins

[Bug 158140748] Add location field on saving

[Bug 158140748] Add decorator and save functions

* [Bug 158140748] Correct flake8 errors

* [Bug 158140748] Correct failing test

* [Bug 158140748] Increase test coverage

* [Bug 158140748] Resolve merge conflicts

* [Bug 158140748] Resolve merge conflicts
add maintainability badge from code-climate to readme
add test-coverage badge from code-climate to readme
add more jobs to config.yml file

[Finishes #158227258]
* [Bug  #158463674] Include calendarId in UpdateRoom mutation

* [Bug  #158463674] Include calendarId in update room mutation test

* [Bug  #158463674] Fix flake8 errors

* #158429512 Delete a room (#44)

* Add untracked  files after merge

* [Fix] Pull Updated changes to develop

* [Feature #158429512] Add Delete room functionality

 This PR adds the delete room functionality where a user can delete a room once they have the correct roomId.This commit also containes tests for this feature.

* #158140748 Restrict room resource deletion (#43)

* [Bug 158140748] Restrict deletion of room resources to admin

[Bug 158140748] Add location for backward relationship

[Bug 158140748] Add block for backward relationship

[Bug 158140748] Add floor for backward relationship

[Bug 158140748] Add room for backward relationship

[Bug 158140748] Add decorator function to restrict mutation for admins

[Bug 158140748] Add location field on saving

[Bug 158140748] Add decorator and save functions

* [Bug 158140748] Correct flake8 errors

* [Bug 158140748] Correct failing test

* [Bug 158140748] Increase test coverage

* [Bug 158140748] Resolve merge conflicts

* [Bug 158140748] Resolve merge conflicts

* chore(code-climate): integrate code-climate with backend (#47)

add maintainability badge from code-climate to readme
add test-coverage badge from code-climate to readme
add more jobs to config.yml file

[Finishes #158227258]

* [Bug  #158463674] Include calendarId in UpdateRoom mutation

* [Bug  #158463674] Include calendarId in update room mutation test

* [Bug  #158463674] Fix flake8 errors

* [Bug  #158463674] Include calendarId in UpdateRoom mutation

* [Bug  #158463674] Include calendarId in update room mutation test

* [Bug  #158463674] Fix flake8 errors

* [Bug  #158463674] Include calendarId in UpdateRoom mutation

* [Bug  #158463674] Include calendarId in update room mutation test

* [Bug  #158463674] Fix flake8 errors

* [Bug  #158463674] Include calendarId in UpdateRoom mutation

* [Bug  #158463674] Include calendarId in UpdateRoom mutation

* [Bug  #158463674] Include calendarId in update room mutation test

* [Bug  #158463674] Include calendarId in update room mutation test

* [Bug  #158463674] Fix flake8 errors

* [Bug  #158463674] Fix flake8 errors

* [Bug 158463674] Rebase with develop

* [Bug 158463674] Rebase with develop

* [Bug 158463674] Refactor schema.py file

* [Bug bg-include-calendar-id-158463674] Add condition for non existant room
@proxiex proxiex requested a review from daddychukz June 28, 2018 12:06
- add a helper method to get user_id from database
- implement the mutation class `DeleteUser` to handle deleting user

- provide relevant user mutation to graphene schema
Copy link

@daddychukz daddychukz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@proxiex proxiex left a comment

Choose a reason for hiding this comment

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

Everything looks good except for the alembic/versions folder, there are way too many of the versions and a lot of them doing nothing. I suggest you get rid of them

@JoyLubega
Copy link
Contributor

@dkam26 Please Remove the Migrations files in The versions folder. If some thing changed in the models, i would advise you add it in the the migration file on develop."initial migration

Copy link
Contributor

@JoyLubega JoyLubega left a comment

Choose a reason for hiding this comment

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

Good Work @dkam26

@PatrickCmd PatrickCmd merged commit 12f48c2 into develop Jul 2, 2018
@shemogumbe shemogumbe deleted the ft-health-check-url-158212394 branch October 29, 2018 11:13
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

10 participants