-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(cluster-connection): implement conterpart for cluster connections api in particular tgw #115
feat(cluster-connection): implement conterpart for cluster connections api in particular tgw #115
Conversation
650ecdb
to
4bd2cc5
Compare
fd0be9b
to
0df4034
Compare
ffa1f8b
to
8969d03
Compare
cidrlist = ["10.201.0.0/16"] | ||
type = "AWS_TGW_ATTACHMENT" | ||
datacenter = "AWS_US_EAST_1" | ||
status = "ACTIVE" |
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 this needed here? this is a computed field that will be set by the provider, can you explain why user would like to provide the status?
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.
Not necessary, removed.
When user creates connection he can pick status between ACTIVE
and INACTIVE
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.
User can create a connection that will be INACTIVE
by default?
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.
Yap
err := resourceClusterConnectionRead(ctx, data, i) | ||
if err != nil { | ||
if errors.Is(err, errNotFound) { | ||
data.SetId("") |
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.
SetId sets the ID of the resource. If the value is blank, then the resource is destroyed.
is the _ = data.Set("cluster_id", 0)
needed in such case?
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.
Question, what to do when resource is gone, while it is in the state? there are three options in this regard:
- Throw an error on
read
. It results in error on nextterraform apply
orterraform refresh
- Silently ignore. It results in having no error on
apply
andrefresh
, but object stays in state and if you want remove/recreate it, you need to go manually remove it from the state - Do
data.SetId("")
and return no error. Will result in having no error onapply
andrefresh
and object been deleted from the state
From my perspective 3rd option produce best UX.
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.
data.Set("cluster_id", 0)
is not needed, removed
if errors.Is(err, errNotFound) { | ||
data.SetId("") | ||
_ = data.Set("cluster_id", 0) | ||
return nil |
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.
why not return the error?
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.
It is going to fail apply
and refresh
operation, which produce bad UX
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.
Why would it be bad UX to fail when reading misconfigured/malformed resource?
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.
It is not malformed, it is deleted resource, on malformed it is going to return error
a510f3d
to
ce70060
Compare
ce70060
to
164300d
Compare
17445c9
to
aca6fd3
Compare
@charconstpointer, could you please take another look after rebase |
examples/resources/scylladbcloud_cluster_connection/aws-tgw-vpc-attachment.tf
Outdated
Show resolved
Hide resolved
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.
👍 LGTM, some less important comments
016519f
to
fd07d74
Compare
fd07d74
to
31dea58
Compare
31dea58
to
081461b
Compare
Closes #121
Cluster connection API was implemented in https://github.com/scylladb/siren/pull/10362. This PR implements terraform counterpart for it.
Tested:
main.tf :
Apply: