internal/controller: ensure $HOME is set for atlas-cli#354
Conversation
giautm
requested review from
chinhnguyenh,
Copilot,
datdao and
luantranminh
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR updates the controller’s Atlas CLI execution environment setup to ensure HOME is configured, and adjusts controller tests to provide a DATA_DIR during runs.
Changes:
- Changed
NewAtlasExecto always deriveHOMEfrom aDATA_DIR(defaulting to/data) and create the directory. - Updated AtlasSchema controller tests to set
DATA_DIRto a temporary directory. - Updated AtlasMigration controller tests to set
DATA_DIRto a temporary directory.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/controller/common.go |
Changes how Atlas CLI environment is prepared by defaulting DATA_DIR and always setting HOME under it. |
internal/controller/atlasschema_controller_test.go |
Sets DATA_DIR in tests to avoid writing to real filesystem locations. |
internal/controller/atlasmigration_controller_test.go |
Sets DATA_DIR in tests to avoid writing to real filesystem locations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dataDir := os.Getenv(envDataDir) | ||
| if dataDir == "" { | ||
| // If DATA_DIR is not set, use /tmp/data | ||
| // as the default directory for storing Atlas CLI data. | ||
| dataDir = "/tmp/data" |
There was a problem hiding this comment.
The function-level comment above still says HOME is only set when DATA_DIR is set, but the updated logic always chooses a dataDir (defaulting to "/tmp/data") and always sets HOME. Please update the comment to match the new behavior and document the default path when DATA_DIR is unset.
| homeDir := filepath.Join(dataDir, home) | ||
| if err := os.MkdirAll(homeDir, 0755); err != nil { | ||
| return nil, fmt.Errorf("creating resource home directory: %w", err) | ||
| } | ||
| env["HOME"] = homeDir |
There was a problem hiding this comment.
This now unconditionally overrides any existing HOME from the inherited environment. Previously, if DATA_DIR was unset, an existing HOME would be preserved. If the goal is only to ensure HOME is non-empty, consider only setting HOME when it’s missing (or make the override explicit via a separate env var/flag) to avoid surprising behavior changes for local/dev runs.
internal/controller/common.go
Outdated
| dataDir = "/tmp/data" | ||
| } | ||
| homeDir := filepath.Join(dataDir, home) | ||
| if err := os.MkdirAll(homeDir, 0755); err != nil { |
There was a problem hiding this comment.
The directory used as Atlas CLI HOME may contain credentials/config under ~/.atlas. Creating it with mode 0755 makes it readable/traversable by other users in the same container/node. Consider using a more restrictive mode (e.g., 0700) for the HOME directory.
| if err := os.MkdirAll(homeDir, 0755); err != nil { | |
| if err := os.MkdirAll(homeDir, 0700); err != nil { |
No description provided.