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 guide for healthchecks #1738

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MetroMarv
Copy link
Contributor

Fixes #1724

[isabell@stardust ~]$ python3.11 -m venv venv
[isabell@stardust ~]$ source venv/bin/activate
(venv) [isabell@stardust ~]$ pip3.11 install -r requirements.txt
Collecting aiosmtpd==1.4.4.post2 (from -r requirements.txt (line 1))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to keep all the log output from that command? Or should I somehow abbreviate it?

Copy link
Member

Choose a reason for hiding this comment

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

That's pretty long, please abbreviate it like in the Kimai Guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Abbreviate it now. Could you resolve the conversation if you like it?


::

(venv) [isabell@stardust ~]$ python3.11 ./manage.py migrate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here (and also in some of the following listings) I assume that the user is still in the active virtualenv. Do you want me to repeat the source venv/bin/activate command before each of the commands requiring the virtualenv?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. If this was my guide I would provide a note before the first use of the venv with something like that:

.. note:: All following commands require the virtualenv, you can activate it with ``source venv/bin/activate`` in a new SSH-session

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefixed all the commands that require a virtual env with (venv). I assume that's clear for every reader.

::

(venv) [isabell@stardust ~]$ python3.11 ./manage.py migrate
System check identified some issues:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again long log output that could be abbreviated.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it makes sense to abbreviate, there is no upside in keeping all of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Abbreviated.

@nichtmax nichtmax self-assigned this Apr 24, 2024
Copy link
Member

@nichtmax nichtmax left a comment

Choose a reason for hiding this comment

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

Thank you for the guide! I left you some comments.

@nichtmax nichtmax marked this pull request as draft April 24, 2024 10:34
@nichtmax nichtmax removed their assignment Apr 24, 2024
@MetroMarv
Copy link
Contributor Author

Thanks for the responses. I'll take care of them at the end of the week!

@nichtmax
Copy link
Member

Any news here? If there's no feedback we'll close the PR.

@MarvinVaillant
Copy link

Hey @nichtmax,
I'll incorporate the discussed points today :)

@MetroMarv MetroMarv force-pushed the 1724/add-guide-for-healthchecks branch from 277b6f6 to 7102fa7 Compare January 19, 2025 13:19
@@ -2,7 +2,7 @@
# INSTALL .: `pre-commit install --overwrite --install-hooks`
# UPDATE ..: `pre-commit autoupdate`
default_language_version:
python: python3.10
python: python3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had apply the changes of #1856 to make it work on my system. And I didn't find a way to work around this. Please let me know if there is any way.

@MetroMarv MetroMarv marked this pull request as ready for review January 19, 2025 14:45
@MetroMarv
Copy link
Contributor Author

I addressed all open conversations now. Could you please have one more look and resolve the conversations?

Thank you very much :)

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.

[healthcheck] Add Guide for healthchecks
3 participants