-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
733ab87
to
212db44
Compare
dc195c9
to
a95569e
Compare
// map. In many cases, there is a field called "name" in the HCL schema, however, | ||
// there are cases like RDS DB Cluster where the name field in HCL is called | ||
// "cluster_identifier". This function is the place that you can take external | ||
// name and assign it to that specific key for that resource type. |
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 would be helpful to explain, perhaps in the ExternalName
comment block, what the distinguishing features of 'name' and 'identifier' are. When can these be the same, when should they differ. Finding that SetIdentifierArgumentFn sets the name
confuses me because identifier
and name
seem to be interchangeable.
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.
Yeah, I agree that it is confusing with such similar words.
May be it should be named as SetNamingArgumentFn
to imply that it is setting the argument that gives the name of the resource?
If that sounds better, may be can consider renaming it (without breaking API of course by marking this one as deprecated and adding the other).
//cc @muvaf
|
||
// GetIDFn returns the string that will be used as "id" key in TF state. In | ||
// many cases, external name format is the same as "id" but when it is not | ||
// we may need information from other places to construct it. For example, |
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.
This adds to the equivocation that I mentioned in https://github.com/crossplane/terrajet/pull/187/files#r777662083
Perhaps a section of the docs needs to be referenced, and in those docs we can go into more detail on the distinctions.
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 missed the example cases describing these scenarios later in the doc.
It may be clearer to discuss the external name vs name vs IDs topic first before introducing these go fields.
After that, the comments in these go fields may still be confusing. Perhaps the type can be offered in these docs without the comments (show the simple struct and link to the source for details). The text and examples in this documentation can describe how the fields can be used, and expand on usage within the scenarios and examples offered.
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 awesome! LGTM after the comments are addressed. Thanks!
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
88ef5f7
to
e3c8435
Compare
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Description of your changes
This PR updates Configuring a Resource guide:
Fixes #153
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
N/A