Skip to content

Improve log file locations#181

Merged
poikilotherm merged 6 commits intodevelopfrom
feature/138-log-location
May 23, 2023
Merged

Improve log file locations#181
poikilotherm merged 6 commits intodevelopfrom
feature/138-log-location

Conversation

@poikilotherm
Copy link
Member

Relates to #138

  1. Make sure the parent directories of the configured log file locations exist (so we can use them) - or else create them
  2. Move default location of hermes log into cache dir

- Make sure the parent directories of the configured log file
  locations exist (so we can use them) - or else create them
- Move default location of hermes log into cache dir
@poikilotherm poikilotherm added the enhancement New feature or request label May 17, 2023
@poikilotherm poikilotherm added this to the Proof-of-concept milestone May 17, 2023
@poikilotherm poikilotherm requested a review from led02 May 17, 2023 14:22
@poikilotherm poikilotherm requested a review from sdruskat May 17, 2023 14:26
@jkelling jkelling self-requested a review May 22, 2023 07:19
Copy link
Contributor

@jkelling jkelling left a comment

Choose a reason for hiding this comment

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

Let us discuss the naming of the --path option. (have to post this comment to remove my approving review.)

@poikilotherm
Copy link
Member Author

poikilotherm commented May 22, 2023

Let us discuss the naming of the --path option. (have to post this comment to remove my approving review.)

I agree we should talk about this. Can we please keep this out of this PR? This one is relevant for properly setting up folks with #110 / #180 for the DataCite meeting. IMHO changing the path command line option seems out of scope for this PR which is only about adapting the logging paths. (Which happens after the CLI option was given, so this is completely independent from how the option is looking like)

@poikilotherm
Copy link
Member Author

@sdruskat you wanted this change so we have a nice oneliner in gitignore. May I ask for your review again so we can ship it in time? Thx!

Copy link
Contributor

@jkelling jkelling left a comment

Choose a reason for hiding this comment

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

The option-name issue is indeed outside of the scope, as the option existed before, I just noticed it here first.

Co-authored-by: jkelling <j.kelling@hzdr.de>
led02
led02 previously requested changes May 23, 2023
Copy link
Member

@led02 led02 left a comment

Choose a reason for hiding this comment

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

Mostly okay, see not for requested change.

@poikilotherm
Copy link
Member Author

I fixed the tests we broke. I will merge now, as all requested changes have been addressed and we need this done for tomorrow. (Usually not drawing that card of overriding our rules.)

@poikilotherm poikilotherm dismissed stale reviews from led02 and jkelling May 23, 2023 13:24

All requests have been addressed.

@poikilotherm poikilotherm merged commit b6986ed into develop May 23, 2023
@poikilotherm poikilotherm deleted the feature/138-log-location branch May 23, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments