bug: cloud-init disable fix #273
Conversation
There was a problem hiding this comment.
Pull Request Overview
Purpose: Fix bug where cloud-init networking disable file creation fails if the target directory is absent by adding a directory existence check and adjusting path handling.
- Add explicit CLOUD_INIT_CONFIG_DIR constant and split full path into dir + filename.
- Check for existence of the config directory before attempting to write, and build the file path with Path::join.
- Update logging around skip and disable actions.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| fn disable_cloud_init_networking() -> Result<(), TridentError> { | ||
| fs::write(CLOUD_INIT_DISABLE_FILE, CLOUD_INIT_DISABLE_CONTENT) | ||
| let cloud_init_disable_path = Path::new(CLOUD_INIT_CONFIG_DIR).join(CLOUD_INIT_DISABLE_FILE); |
There was a problem hiding this comment.
nit: You could move this further down post the check.
| fn disable_cloud_init_networking() -> Result<(), TridentError> { | ||
| fs::write(CLOUD_INIT_DISABLE_FILE, CLOUD_INIT_DISABLE_CONTENT) | ||
| let cloud_init_disable_path = Path::new(CLOUD_INIT_CONFIG_DIR).join(CLOUD_INIT_DISABLE_FILE); | ||
| if !Path::new(CLOUD_INIT_CONFIG_DIR).exists() { |
There was a problem hiding this comment.
I think this is sufficient to address this bug, but should we have a better handling re package presence detection in general? We could use the cosi manifest now, right?
There was a problem hiding this comment.
Supposedly yes, but i think i want to add some cleaner interfaces for that before we cross that bridge
| } | ||
| } | ||
|
|
||
| fn disable_cloud_init_networking() -> Result<(), TridentError> { |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if !config_dir.as_ref().exists() { | ||
| debug!( | ||
| "Cloud-init config dir {} does not exist, skipping disabling cloud-init networking", | ||
| config_dir.as_ref().display() | ||
| ); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| debug!("Disabling cloud-init networking"); | ||
| let cloud_init_disable_path = config_dir.as_ref().join(CLOUD_INIT_DISABLE_FILE); |
There was a problem hiding this comment.
[nitpick] config_dir.as_ref() is called multiple times; storing it in a local variable (e.g., let config_dir = config_dir.as_ref();) would improve readability and avoid repetition.
| if !config_dir.as_ref().exists() { | |
| debug!( | |
| "Cloud-init config dir {} does not exist, skipping disabling cloud-init networking", | |
| config_dir.as_ref().display() | |
| ); | |
| return Ok(()); | |
| } | |
| debug!("Disabling cloud-init networking"); | |
| let cloud_init_disable_path = config_dir.as_ref().join(CLOUD_INIT_DISABLE_FILE); | |
| let config_dir = config_dir.as_ref(); | |
| if !config_dir.exists() { | |
| debug!( | |
| "Cloud-init config dir {} does not exist, skipping disabling cloud-init networking", | |
| config_dir.display() | |
| ); | |
| return Ok(()); | |
| } | |
| debug!("Disabling cloud-init networking"); | |
| let cloud_init_disable_path = config_dir.join(CLOUD_INIT_DISABLE_FILE); |
🔍 Description
Fix #272
Check if directory exists before attempting to create file.