-
Notifications
You must be signed in to change notification settings - Fork 67
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: machine proxy resource and data source #809
Conversation
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.
Looks good 👍
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, my only concern is ensuring that none of the operations allow a proxy password to be retrievable through the TF State. I assume this has been covered, it's just not super-clear from reading the code.
}) | ||
} | ||
|
||
data.ID = types.StringValue(fmt.Sprintf("Proxies-%s", time.Now().UTC().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.
What does this do for TF state if, for example, the datasource was directly set as the value of an output? Do we do this form of ID'ing in other datasources?
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.
That is how terraform recommends handling state for data sources, we do it in all our data sources. This ID is internal for terraform, it uses it to understand and track when the data source changes.
return | ||
} | ||
|
||
machineProxy, err := proxies.GetByID(r.Client, state.SpaceID.ValueString(), state.ID.ValueString()) |
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.
I'm sure it's handled, but just for my clarity: if the user doesn't provide a SpaceId on the Datasource, will this state.SpaceID
be automatically set to the default Space from the Provider config?
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, that happens inside the Go client, if the space ID is "" (go will not let it be nil) we will use the space provided on the provider, if one is not provided by the provider it will use default space, if you don't have a default space set it will error gracefully.
|
||
updatedProxy := mapMachineProxyModelToRequest(&plan) | ||
updatedProxy.ID = existingProxy.ID | ||
updatedProxy.Links = existingProxy.Links |
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.
Eww :) do we have to? I assume so, but would love if TFP didn't need to concern itself with this detail.
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.
Same. At least its not exposed to user.
Machine proxy resource and datasource.
closes #696