Export configuration information in shell launched by Atmos #827
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
internal/exec/shell_utils.go (1)
265-267: Convert comment into actionable enhancement for Geodesic detection.The comment identifies a valid compatibility issue with custom shell prompts in Geodesic. Let's track this as a feature enhancement.
Would you like me to create a GitHub issue to track the implementation of Geodesic shell detection? The issue would include:
- Detection of Geodesic environment using
GEODESIC_SHELLenvironment variable- Special handling of shell prompt configuration when running inside Geodesic
- Documentation updates for Geodesic compatibility
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/shell_utils.go(2 hunks)
🔇 Additional comments (2)
internal/exec/shell_utils.go (2)
188-201: LGTM! The implementation successfully achieves the PR objectives.
The changes effectively export Terraform configuration information when launching a shell, which will help users maintain awareness of their environment state. The implementation is clean and well-documented.
188-193: Consider adding validation for the varFile parameter.
The code consistently sets up Terraform CLI arguments for all relevant commands. However, it might be worth adding validation to ensure the varFile exists and is readable before setting these environment variables. This would help prevent shell initialization with invalid configurations.
Let's check if similar validation exists elsewhere in the codebase:
|
We should also update the docs to describe this functionality. The best I can think of is adding it here, in a new section. Ideally we should document all environment variables that we set. https://atmos.tools/cli/configuration/#environment-variables |
Nuru
force-pushed
the
export-shell-envs
branch
from
f874777 to
573b321
Compare
@osterman documentation added |
Nuru
dismissed
coderabbitai[bot]’s stale review
Requested change is beyond the scope of this PR
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
website/docs/cli/configuration/configuration.mdx (1)
637-653: Fix style and typo in the context section.The section provides valuable information about environment variables in spawned shells, but needs minor refinements:
- Line 640: Consider rephrasing "in order to enable" to "to enable" for better readability
- Line 651: Fix typo "comamnds" to "commands"
Apply this diff to fix the issues:
-spawn an interactive shell with certain environment variables set, in order to enable the user to use other tools +spawn an interactive shell with certain environment variables set to enable the user to use other tools -| ATMOS_TERRAFORM_WORKSPACE | The name of the Terraform workspace in which Terraform comamnds should be run | +| ATMOS_TERRAFORM_WORKSPACE | The name of the Terraform workspace in which Terraform commands should be run |🧰 Tools
🪛 LanguageTool
[style] ~640-~640: Consider a shorter alternative to avoid wordiness.
Context: ...with certain environment variables set, in order to enable the user to use other tools (in ...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
internal/exec/shell_utils.go(2 hunks)website/docs/cli/configuration/configuration.mdx(2 hunks)website/docs/quick-start/install-atmos.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/shell_utils.go
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/configuration/configuration.mdx
[style] ~640-~640: Consider a shorter alternative to avoid wordiness.
Context: ...with certain environment variables set, in order to enable the user to use other tools (in ...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (2)
website/docs/quick-start/install-atmos.mdx (1)
312-312: LGTM! Good addition to the documentation.
The tip about recommending "Fira Code" NerdFont version is helpful for users to achieve consistent terminal display.
website/docs/cli/configuration/configuration.mdx (1)
Line range hint 602-636: LGTM! Comprehensive documentation of environment variables.
The table provides clear mapping between environment variables and their corresponding YAML paths, making it easier for users to understand configuration options.
|
These changes were released in v1.120.0. |
|
@coderabbitai Please create a GitHub issue to track the implementation of Geodesic shell detection |
|
Tip For best results, initiate chat on the files or code changes. @Nuru, I've created the GitHub issue to track the implementation of Geodesic shell detection: #837. 🔗 Analysis chain@Nuru, I'll create a GitHub issue to track the implementation of Geodesic shell detection. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 294 |
what
why
Summary by CodeRabbit
New Features
Documentation
atmos.yamlconfiguration file description and loading priority details.