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

Add Health endpoint #140

Merged
merged 4 commits into from
Jun 4, 2024
Merged

Conversation

xiomaraR
Copy link
Collaborator

@xiomaraR xiomaraR commented May 29, 2024

Description

This pull request introduces the addition of a health endpoint along with related tests. It also includes modifications to the authentication middleware so that the health endpoint is bypassed.

Commit type

  • feat: indicates the addition of a new feature or functionality to the project.

Issue

Create health endpoint #134

Solution

The health endpoint is added to provide a health check for the application, which can be useful for monitoring purposes. The modifications to bypass authentication for the health endpoint keeps it simple and accessible.

Proposed Changes

-Added health endpoint router and related tests
-Included health router in __main__.py
-Modified authentication middleware to bypass authentication for health endpoint
-Utilized Starlette's named constants for status codes

Potential Impact

These changes are expected to have a positive impact on the project by enhancing its monitoring capabilities.

Tests Performed

  • [✅ ] Test performed

Additional Tasks

-Review and address any feedback received during the pull request review process

Assigned

@AntonioMrtz

@xiomaraR
Copy link
Collaborator Author

This PR replaces #139

@AntonioMrtz AntonioMrtz self-requested a review May 29, 2024 06:45
Copy link
Owner

@AntonioMrtz AntonioMrtz left a comment

Choose a reason for hiding this comment

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

Hi, first of all thanks for contributing to the project and taking the time to solve all the issues you had with the setup.

Its recommend in PRs that the only changes included should be related to the issue. Some changes are out of scope:

  • Imports re-formatting and changing alias
  • Non related files changes such as .env.example, docker scripts

The overall PR is really good, the health endpoint was correct, tests were good and health endpoint was added into middleware bypass paths.

For the PR to be approve we have to fix the changes that were out of this scope for this PR. I will review them when there finished.

Once more, thanks for your work. If you have any question related to the project or the review just ask me. The PR is good and only minor changes have to be made. Thanks for your time :)

Backend/.env.local Outdated Show resolved Hide resolved
Backend/tests/test_health.py Outdated Show resolved Hide resolved
Backend/tests/test__user.py Outdated Show resolved Hide resolved
Backend/docker/build_and_up_dev.sh Outdated Show resolved Hide resolved
Backend/app/__main__.py Outdated Show resolved Hide resolved
Backend/app/routers/health.py Outdated Show resolved Hide resolved
Backend/app/routers/generos.py Outdated Show resolved Hide resolved
Backend/app/routers/canciones.py Outdated Show resolved Hide resolved
Backend/app/routers/artistas.py Outdated Show resolved Hide resolved
Backend/app/middleware/CheckJwtAuthMiddleware.py Outdated Show resolved Hide resolved
@AntonioMrtz AntonioMrtz linked an issue May 29, 2024 that may be closed by this pull request
@xiomaraR
Copy link
Collaborator Author

@AntonioMrtz thanks for the feedback, I'll try to make the changes suggested ASAP. The import changes for those files you mentioned happened after running the style and linting on the code. I wasn't sure whether to revert it or not at the time

…e, reverted login.py to commit 3ef4f8c, reverted build_and_up_dev.sh to commit 3ef4f8c, modified magic numbers in health route files
@xiomaraR
Copy link
Collaborator Author

xiomaraR commented May 29, 2024

@AntonioMrtz I made your suggested changes. I used the --no-verify flag when committing because one of the pre-commit hooks altered the imports again, despite my efforts to revert them to their original state. This was the only solution I could think of. Let me know if I should have done anything differently!

@AntonioMrtz
Copy link
Owner

@AntonioMrtz I made your suggested changes. I used the --no-verify flag when committing because one of the pre-commit hooks altered the imports again, despite my efforts to revert them to their original state. This was the only solution I could think of. Let me know if I should have done anything differently!

Hi, using --no-verify flag its a good solution. I will try to have the refactored version ASAP. This will make the overall development easier.

@AntonioMrtz AntonioMrtz self-requested a review May 30, 2024 11:19
Copy link
Owner

@AntonioMrtz AntonioMrtz left a comment

Choose a reason for hiding this comment

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

Rollback UTC changes :)

@AntonioMrtz
Copy link
Owner

Once youre finished, can you update the PR description with the final changes? Thanks in advance.

Copy link
Owner

@AntonioMrtz AntonioMrtz left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. Good PR, i hope it helped you understand how is our workflow and motivates you to keep contributing to the project. There are some issues both for backend and frontend and when i have time i will publish more that i have stored on Trello. I hope your contributing experience was great. If it seems good to you i would be eager to see more of your work in the project. This issue shouldnt take long, take a look if u want. Thanks again and i hope to see you working on the project soon, I will add your github profile to the contributors lists. Regards :)

@AntonioMrtz AntonioMrtz merged commit a5c2fb0 into AntonioMrtz:master Jun 4, 2024
@xiomaraR xiomaraR deleted the feat/health-endpoint branch June 6, 2024 19:49
@AntonioMrtz AntonioMrtz changed the title Feat: health endpoint Add Health endpoint Jul 2, 2024
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.

Create health endpoint
2 participants