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

Issue #929: logging: Ensure ~/.crc exists before creating log file #932

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

cfergeau
Copy link
Contributor

crc-embedder currently fails if ~/.crc does not exist. This is because
it tries to create a log file before ~/.crc is created. Adding a call to
EnsureBaseDirExists() before trying to create the log file fixes this
issue.

This fixes #929

…file

crc-embedder currently fails if ~/.crc does not exist. This is because
it tries to create a log file before ~/.crc is created. Adding a call to
EnsureBaseDirExists() before trying to create the log file fixes this
issue.

This fixes crc-org#929
@praveenkumar
Copy link
Member

retest this please.

err := constants.EnsureBaseDirExists()
if err != nil {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

@cfergeau This should be part of crc-embedder/root.go script instead here, same way it is part of crc/root.go because it is not only creating the log directory but initial structure so should be part of init method of root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With Anjan's refactor of this code, we can move the responsibility of creating the directories to the caller.
With the current code, where it's pkg/crc/logging/logging.go which decides where to put its log file, to me it makes sense that it also makes sure the directory exists.

Copy link
Member

Choose a reason for hiding this comment

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

@cfergeau you mean 1f9070d this refactor? This is just adding a path argument, or it is a different one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this one. Currently, where the log is stored is an implementation detail of the 'logging' package, so in my opinion it makes sense that the logging package takes care of creating the path.
After Anjan's refactor, where the log is stored is specified by users of the package iirc. The location of the log file is no longer internal to the logging package, so it makes sense that users of logging are required to make sure the path exists.

@praveenkumar praveenkumar merged commit 8b35135 into crc-org:master Jan 21, 2020
@cfergeau cfergeau deleted the issue_929 branch October 19, 2020 12:51
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.

[BUG] make release fails if ~/.crc doesn't exist
2 participants