Skip to content

log/scripts: introduce proper log level fallback and env getter function#2506

Merged
georglauterbach merged 9 commits intomasterfrom
scripts/log/proper-fallback
Apr 5, 2022
Merged

log/scripts: introduce proper log level fallback and env getter function#2506
georglauterbach merged 9 commits intomasterfrom
scripts/log/proper-fallback

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Mar 26, 2022 •
edited
Loading

Description

This PR does two small things:

  1. The log level, in case it is unset, will now be "calculated" from
    /etc/dms-settings and not always default to info. This way, we
    can ensure that more often than not, the log level the user chose
    when starting DMS is used everywhere.
  2. I noticed that the way I obtained the log level could be used to
    obtain any env variable's value. I therefore added a function to
    utils.sh in case we use it in the future.

The reason for not using the new function from utils.sh in log.sh is that I would like to preserve log.sh's property of not being dependent on other helper scripts.

(There is a typo in the commit message: obtain any env variable's log level -> obtain any env variable's value - don't get confused 😉)

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@georglauterbach
introduce proper log level fallback and env getter function
23ab211 
This PR does two small things:

1. The log level, in case it is unset, will now be "calculated" from
   `/etc/dms-settings` and not always default to `info`. This way, we
   can ensure that more often than not, the log level the user chose
   when starting DMS is used everywhere.
2. I noticed that the way I obtained the log level could be used to
   obtain any env variable's log level. I therefore added a function to
   `utils.sh` in case we use it in the future.
@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Mar 26, 2022
@georglauterbach georglauterbach added this to the v11.0.0 milestone Mar 26, 2022
@georglauterbach georglauterbach self-assigned this Mar 26, 2022
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Mar 26, 2022

Not sure whether there actually is a use case for the new function in utils.sh. When I just thought about it, this would only come in handy when

  1. /etc/dms-settings is present
  2. but there is a reason we cannot just source it

Does this even happen?

@polarathene
Copy link
Copy Markdown
Member

polarathene commented Mar 26, 2022

Does this even happen?

/etc/dms-settings is fairly new, besides the recent decision to source it in check-for-changes.sh only a few CLI utilities source it AFAIK? Often even when we're only interested in a few select vars.

In check-for-changes.sh however, those values are specifically needed in helper scripts themselves. Technically those scripts could run this utility method instead as a way to "import" their global/external ENV var dependency, and perhaps log an error/warning if empty? (unless there is a fallback/default given).

That might be a better way to safe-guard against maintenance changes shuffling/splitting stuff around across files or importing specific helpers (although some of these may also depend on methods from other helpers... they were all originally introduced as importing index.sh to get everything including indirectly the one you want, but I've seen you've been sourcing specific helpers directly as of late..).

polarathene
polarathene previously approved these changes Mar 26, 2022
View reviewed changes
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Mar 27, 2022 •
edited
Loading

That might be a better way to safe-guard against maintenance changes shuffling/splitting stuff around across files or importing specific helpers (although some of these may also depend on methods from other helpers... they were all originally introduced as importing index.sh to get everything including indirectly the one you want, but I've seen you've been sourcing specific helpers directly as of late..).

Yes, we need to take care what to import and where to import. We will need to prevent a "sourcing hell" where we're not sure what is already sourced and what is not. I think I have a solution. I will provide this solution and the appropriate adjustments in a separate PR.

EDIT: Actually, we're quite fine as of now when it comes to importing helpers. We're only sourcing index.sh or log.sh which is completely fine.

casperklein
casperklein reviewed Apr 1, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@casperklein casperklein left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Disclaimer: I am a bit in a hurry and did not read all comments here (will do this later/tomorrow).

I am not sure/understand, what exactly is tried to solve here in this PR. Can someone maybe give an example?

Comment thread target/scripts/helpers/utils.sh Outdated
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 2, 2022

I am not sure/understand, what exactly is tried to solve here in this PR. Can someone maybe give an example?

I was worried in case LOG_LEVEL is not set in the environment we'd always just fall back to info. I thought this was not desired, and therefore I'd first check if we can parse LOG_LEVEL from /etc/dms-settings.

polarathene
polarathene previously approved these changes Apr 2, 2022
View reviewed changes
@georglauterbach georglauterbach mentioned this pull request Apr 4, 2022
Merged
8 tasks
casperklein
casperklein reviewed Apr 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

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

One last minor thing I noticed. Otherwise LGTM.

Comment thread target/scripts/helpers/log.sh Outdated
@georglauterbach georglauterbach merged commit b1594a8 into master Apr 5, 2022
@georglauterbach georglauterbach deleted the scripts/log/proper-fallback branch April 5, 2022 15:10
@polarathene polarathene mentioned this pull request Aug 31, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein approved these changes

@polarathene polarathene polarathene approved these changes

@wernerfred wernerfred Awaiting requested review from wernerfred

Assignees

@georglauterbach georglauterbach

Labels

area/scripts kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Milestone

v11.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@georglauterbach @polarathene @casperklein