feat(DEV-2936): Spinner for !terraform.output
#947
Conversation
milldr
added
minor
tests/fixtures/scenarios/atmos-functions/stacks/catalog/component-1.yaml
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Looks like it's failing on: |
Yes I know what needs to be fixed (handling TTY), but havent had time yet. Will get to it as soon as I can |
milldr
force-pushed
the
DEV-2936/terraform-output-spinner
branch
from
9a0de76 to
6dfb0c4
Compare
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the Atmos project's infrastructure, focusing on improving the Terraform output retrieval process and configuration management. The changes span multiple files, including Changes
Sequence DiagramsequenceDiagram
participant User
participant AtmosCLI
participant TerraformComponent
participant Spinner
User->>AtmosCI: Request Terraform output
AtmosCI->>Spinner: Initialize spinner
Spinner->>AtmosCI: Spinner running
AtmosCI->>TerraformComponent: Retrieve outputs
TerraformComponent-->>AtmosCI: Return outputs
AtmosCI->>Spinner: Stop spinner
Spinner->>User: Display results
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/fixtures/components/terraform/mock/main.tf (2)
1-4: Variable declarations.
Having defaults is sensible for testing. Consider adding descriptions to clarify usage.
6-9: Combining variables.
These string variables appear uniform. You might combine or programmatically set them if they share logic.Also applies to: 11-14
tests/fixtures/scenarios/atmos-functions/atmos.yaml (1)
10-10: Remove Trailing WhitespaceThere's trailing whitespace at the end of this blank line. It's best to remove it to maintain a cleaner YAML file.
Apply this diff to remove the trailing whitespace:
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
tests/test-cases/atmos-functions.yaml (1)
9-9: Correct Indentation and Add NewlineMinor YAML formatting issues:
- Lines 9 & 33 have an extra space.
- There's no newline character at the end of the file.
Apply this diff to fix both:
- - "terraform" + - "terraform" - - "terraform" + - "terraform" - - "^$" \ No newline at end of file + - "^$" +Also applies to: 33-33, 45-45
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 9-9: wrong indentation: expected 6 but found 7
(indentation)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)Makefile(1 hunks)internal/exec/spinner_utils.go(1 hunks)internal/exec/terraform_outputs.go(3 hunks)tests/fixtures/components/terraform/mock/main.tf(1 hunks)tests/fixtures/scenarios/atmos-functions/atmos.yaml(1 hunks)tests/fixtures/scenarios/atmos-functions/stacks/deploy/nonprod.yaml(1 hunks)tests/fixtures/scenarios/atmos-functions/stacks/workflows/terraform-output.yaml(1 hunks)tests/test-cases/atmos-functions.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/fixtures/scenarios/atmos-functions/atmos.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
tests/test-cases/atmos-functions.yaml
[warning] 9-9: wrong indentation: expected 6 but found 7
(indentation)
[warning] 33-33: wrong indentation: expected 6 but found 7
(indentation)
[error] 45-45: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (23)
internal/exec/spinner_utils.go (6)
1-2: File creation acknowledged.
This new package-level file seems well-scoped for spinner logic.
3-8: Import references look good.
The bubbletea/spinner imports are properly grouped, promoting clarity.
10-13: Struct design note.
Using a value receiver here is acceptable for bubbletea’s update loop. Ensure all fields that need mutation are consistently updated through return statements.
15-17: Init function clarity.
Returningm.spinner.Tickstraightforwardly starts the spinner. This is consistent with bubbletea patterns.
19-35: Update function approach.
The switch-case logic is concise and gracefully handles key interrupt signals. Good job stopping on "ctrl+c" or "q".
37-39: View function format.
Printing "\r" for carriage return can behave differently on some terminals. Usually works fine, but keep an eye out for cross-platform quirks.internal/exec/terraform_outputs.go (11)
14-15: Spinner dependency.
Great to see bubbletea/spinner integrated for user feedback.
221-221: Message string.
The message clarifies which component and stack are queried. This is helpful for debugging.
232-235: Spinner initialization.
Setting a style fromtheme.Styles.Linkis consistent with the rest of the UI approach.
236-242: Non-TTY fallback.
well done gracefully handling no TTY scenario. The fallback to plain output helps in CI environments.
244-248: Program creation logic.
This approach is straightforward, passing the spinner state into the program with optional TTY.
249-258: Goroutine usage.
Spinning a separate goroutine forp.Runallows concurrent Terraform actions. Confirm no concurrency race on shared data.
261-263: Shutdown on error.
Callingp.Quit()before printing the error is good. Make sure to always await the channel so the goroutine ends.
271-273: Additional error path.
Same pattern used for remote state error. Consistency is beneficial.
277-281: Outcome of static outputs.
The logic storing results in the cache is consistent with the rest of the code.
286-288: Error-stop synergy.
Again,p.Quit()ensures the spinner is halted. This is well-handled.
294-302: Final success display.
Terminating the spinner with a checkmark is intuitive. Good user feedback.tests/fixtures/scenarios/atmos-functions/stacks/workflows/terraform-output.yaml (1)
1-8: Workflow is straightforward.
Sequentially deploying each component helps test cross-component outputs. This approach is clear.tests/fixtures/components/terraform/mock/main.tf (1)
16-18: Output usage.
Providing outputs for each variable is practical for testing!terraform.output. Good coverage.Also applies to: 20-22, 24-26
tests/fixtures/scenarios/atmos-functions/atmos.yaml (1)
1-21: Configuration Looks GoodThe overall structure and keys (components, stacks, and logs) are clear and match the intended usage. Great job keeping this organized.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
.gitignore (1)
37-43: Well-Structured Ignore PatternsThese newly added patterns effectively ignore the specified cache and vendor paths, helping keep the repository clean.
tests/fixtures/scenarios/atmos-functions/stacks/deploy/nonprod.yaml (1)
1-31: Non-Prod Stack Configuration is ClearThe file properly outlines non-production components, each with distinct variables. This approach of referencing outputs across components is a solid practice, demonstrating DRY usage through
!terraform.output.Makefile (1)
58-58: Longer Test Timeout is ReasonableExtending the timeout to 10 minutes is sensible for complex Terraform operations and spinner interactions.
|
These changes were released in v1.154.0. |
what
!terraform.outputterraform_outputs_model.gowhy
references
Summary by CodeRabbit
Release Notes
New Features
Testing
Configuration
.gitignoreto exclude cache and vendor directoriesPerformance