-
Notifications
You must be signed in to change notification settings - Fork 239
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
[Spike] Use crc config
to use custom location for .crc directory
#4088
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR is related to #3966 |
That should be in the first message, as currently it is just the template. |
- Add a new config key called crc-dir - Initialise Crc base directory variables based on the saved config - New config will take effect after rerunning `crc setup`
crc config
to use custom location for .crc directory
Reading the issue/PR here I wonder how this could work. as the config itself lives inside the .crc folder.
Doing a Using an environment variable also makes this a more deliberate choice, as there are side effects for sure. |
Thinking out loud is always helpful, especially in the case when, like in this issue, the situation is not clearly described. It becomes more of a trial and error. Almost like a spike issue. Writing down some things before working on this might help with the actual implementation. I, for example, immediately think of a chicken-egg problem when using this as a configurable from the config itself, but also, what about the long-running processes? They need to be restarted? Also, for a |
@vyasgun: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
I haven't looked at the implementation yet, but what would happen when the location is changed after the VM is already created? One of the things we do not perform before the start is to check if all the VM files are available. This might be something that has to exist regardless of this change, but due to the possibility to change the |
@@ -37,6 +37,7 @@ const ( | |||
EmergencyLogin = "enable-emergency-login" | |||
PersistentVolumeSize = "persistent-volume-size" | |||
EnableBundleQuayFallback = "enable-bundle-quay-fallback" | |||
CrcDir = "crc-dir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the config directory or crc directory?
@@ -167,6 +173,10 @@ func GetPreset(config Storage) preset.Preset { | |||
return preset.ParsePreset(config.Get(Preset).AsString()) | |||
} | |||
|
|||
func GetConfigDir(config Storage) string { | |||
return config.Get(CrcDir).AsString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the question: "Is this the config directory or crc directory?"
@@ -175,6 +176,26 @@ func EnsureBaseDirectoriesExist() error { | |||
return nil | |||
} | |||
|
|||
func initialiseAllDirectories(crcDir string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not just about directories, as there are also sockets, and configuration files.
Overall the changes make sense, especially to comnfirm the permissions with the validate. However, as mentioned above, more can be at fault. Edge cases exist when a VM is already created; in that case a start could result in an error of an already existing VM by name, but not in the 'new' machines folder. Please try some of these scenarios to see what I mean. A possible solution would be to prevent changing this folder when a VM is currently registered. |
crc config
to use custom location for .crc directorycrc config
to use custom location for .crc directory
@@ -168,6 +168,16 @@ func ValidatePath(path string) error { | |||
return nil | |||
} | |||
|
|||
// ValidateDirectory checks if provided path exists and has sufficient permissions | |||
func ValidateDirectory(path string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great for re-use
The objective of this spike was to test whether the
Next steps:
|
Setting the location using |
Fixes: Issue #3966
Solution/Idea
The location for .crc directory is made configurable for the user through
crc config
.Currently, this PR will use the value set in the config as the new base directory.
Proposed changes
List main as well as consequential changes you introduced or had to introduce.
crc-dir
crc setup
Testing
What is the bottom-line functionality that needs testing? Describe in pseudo-code or in English. Use verifiable statements that tie your changes to existing functionality.