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

Reduced sourcecode handler #367

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Reduced sourcecode handler #367

merged 3 commits into from
Feb 1, 2023

Conversation

CommanderStorm
Copy link
Member

Proposed Changes (include Screenshots if possible)

  • This PR removes the source code handlers
  • Reasoning:
    • For debugging, I don't consider this useful
    • Why keep something that is not useful?

How to test this PR

  1. Discuss if sourcecode is not needed instead

How has this been tested?

  • internal monolouge, that this feature is not needed

Checklist:

  • I have updated the documentation / No need to update the documentation
  • I have run the linter

@CommanderStorm CommanderStorm added the server Related to the backend/server label Jan 27, 2023
@CommanderStorm CommanderStorm self-assigned this Jan 27, 2023
@octycs
Copy link
Contributor

octycs commented Jan 27, 2023

Since it's GPL (as said in the doc), we don't need to have it. For me it's kind of equal whether to keep it or to remove it. It follows the idea of GPL to provide access to the source code (so it wasn't intended for debugging anyway), but the repo is also directly accessible from the NavigaTUM website. The only thing is that people without access to the deployment might not know which commit is currently active. But if you say it's rather unnecessary or even standing in the way, I am okay to remove it.

@CommanderStorm CommanderStorm marked this pull request as draft January 28, 2023 00:11
@CommanderStorm
Copy link
Member Author

CommanderStorm commented Jan 28, 2023

The only thing is that people without access to the deployment might not know which commit is currently active.

Normally I would respond that this never happens and if it does it would be my job to spot this via the alerting we have.
But #352 has shown me some gaps in my alerting for which I really dont have a good way to adress them, so you are probably right.
Thinking more about this, I think this should be combining the health and the source_code endpoint into a status endpoint.
Such an endpoint could respond with:

healthy
running: <github link>

@CommanderStorm CommanderStorm changed the title Removed the sourcecode handler sourcecode handler Jan 28, 2023
@CommanderStorm CommanderStorm changed the title sourcecode handler Reduced sourcecode handler Jan 28, 2023
@CommanderStorm CommanderStorm marked this pull request as ready for review January 28, 2023 01:28
server/src/main.rs Outdated Show resolved Hide resolved
@octycs
Copy link
Contributor

octycs commented Jan 29, 2023

Thinking more about this, I think this should be combining the health and the source_code endpoint into a status endpoint.

That's a good compromise 👍

@CommanderStorm CommanderStorm merged commit 8af0c49 into main Feb 1, 2023
@CommanderStorm CommanderStorm deleted the no_sourcecode_handler branch February 1, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the backend/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants