-
Notifications
You must be signed in to change notification settings - Fork 102
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
TF-18548: add stack API necessary for CLI #940
Conversation
a233d43
to
648dd63
Compare
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.
Your changes look good :) I left some comments. And also I'd like to request that you update the changelog.md to make a note of your changes. Thanks!
} | ||
|
||
type ChangeAction 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.
Can you add a small comment/description for all the types that don't have one on top? This information is used by our godoc.
@@ -92,6 +96,67 @@ type StackPlan struct { | |||
Stack *Stack `jsonapi:"relation,stack"` | |||
} | |||
|
|||
// JSONChangeDesc represents a change description of a stack plan / apply operation. | |||
type JSONChangeDesc struct { |
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 it expected that JSONChangeDesc and related structs are using json tags instead of jsonapi?
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.
Yes, we are not reading an API response, but a JSON document stored in archivist (https://github.com/hashicorp/atlas/blob/8f60a5696d441acaa53dc3387184561b7e1932d8/app/models/stack_plan.rb#L82-L86)
stack_plan.go
Outdated
Actions []ChangeAction `json:"actions"` | ||
After json.RawMessage `json:"after"` | ||
Before json.RawMessage `json:"before"` | ||
// TODO: Add after_sensitive, after_unknown, before_sensitive |
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.
At what point can these be added?
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.
Turned out these are computed based on the go-cty values in the before and after message, deleting the TODO :)
I decided to not unmarshal this here since moving the type from inside terraform to here seems overkill.
Co-authored-by: Luces Huayhuaca <[email protected]>
Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes. |
Description
This PR adds support for a few APIs the stacks CLI needs to display a remote plan
Testing plan
For now the integration tests should be sufficient
External links
Output from tests
Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.