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

Refactor logging in start.sh and add JUPYTER_DOCKER_STACKS_QUIET #1526

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Nov 11, 2021

Cherry-pick of 63295ba, working through #1512 (comment)

It is meant to allow you to opt out from non-error non-warning logs
generated by start.sh.

@maresb maresb force-pushed the quiet branch 2 times, most recently from f974efe to a59c3bb Compare November 12, 2021 11:33
consideRatio and others added 2 commits November 12, 2021 12:35
It is meant to allow you to opt out from non-error non-warning logs
generated by start.sh.
Make noise when JUPYTER_DOCKER_STACKS_QUIET is empty
Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

Looks awesome:

Possible future improvements (just a note for someone, who will want to improve our docker stacks):

  1. Always add log_level like this: `_log "INFO" "my_message"
  2. Based on log level color can be added.
  3. Exit on fatal errors (or maybe just errors)
  4. Add timestamps, like INFO[2021-08-23 11:00:00.000000] my_message
  5. Stop using bash, haha 👍

@maresb maybe you could add a test for this variable?

@mathbunnyru
Copy link
Member

@maresb
I added an issue to improve log function, I think, if you added a test, then it would be perfect.
Another thing to add is a line-or-two of documentation.

@maresb
Copy link
Contributor Author

maresb commented Nov 15, 2021

  1. Stop using bash, haha 👍

⬆️ ✔️ ⬆️ ✔️ ⬆️ ✔️

I'm convinced that this right here is the root of most of our problems.

If we get a comprehensive test suite for start.sh running, I think it'd be a piece of cake to introduce #1233 and bring it up to feature parity. Once we're convinced it's reliably working, then ditch start.sh and maintenance and features become so much easier and more sane.

@maresb
Copy link
Contributor Author

maresb commented Nov 15, 2021

Hmm, seems to be a CI error, I'll restart.

@maresb maresb closed this Nov 15, 2021
@maresb maresb reopened this Nov 15, 2021
@mathbunnyru
Copy link
Member

@maresb @consideRatio I gave this a thought - do we really need JUPYTER_DOCKER_STACKS_QUIET?
I think this script generates only a couple of lines of logs in the worst case and it's not a big deal.

I personally think it's easier to remove this variable and merge this PR without adding a test case.
If someone will need this variable, then we will gladly accept the PR in the future.

@mathbunnyru mathbunnyru changed the title Add JUPYTER_DOCKER_STACKS_QUIET Refactor logging in start.sh and add JUPYTER_DOCKER_STACKS_QUIET Nov 15, 2021
@consideRatio
Copy link
Collaborator

consideRatio commented Nov 15, 2021

I recall adding it initially because there was some noise about "if this emits logs, it means my automation runs into trouble that parses the output" or similar. I've lost track of the change motivation at this point, but I recall it wasn't stemming from solving a problem of my own.

Any outcome is fine by me, but I figure for anyone using this to run a quick command in the container separate from starting the main process or similar, wanting to inspect the output of that command, this does make some sense.

@mathbunnyru
Copy link
Member

I updated the issue, let's merge this 👍

@mathbunnyru mathbunnyru merged commit d616811 into jupyter:master Nov 16, 2021
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.

3 participants