Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Update Configuring a Resource guide #187

Merged
merged 4 commits into from
Jan 7, 2022
Merged

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Dec 29, 2021

Description of your changes

This PR updates Configuring a Resource guide:

  • For latest changes in External name configuration
  • Adds a section for overriding Terraform schema

Fixes #153

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

N/A

@turkenh turkenh changed the title Update resource config guide for external name Update resource config guide Dec 30, 2021
@turkenh turkenh changed the title Update resource config guide Update Configuring a Resource guide Dec 30, 2021
// 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.
Copy link
Member

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.

Copy link
Member Author

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,
Copy link
Member

@displague displague Jan 3, 2022

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.

Copy link
Member

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.

Copy link
Member

@muvaf muvaf left a 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!

docs/configuring-a-resource.md Outdated Show resolved Hide resolved
docs/configuring-a-resource.md Show resolved Hide resolved
docs/configuring-a-resource.md Outdated Show resolved Hide resolved
docs/configuring-a-resource.md Show resolved Hide resolved
docs/configuring-a-resource.md Outdated Show resolved Hide resolved
@turkenh turkenh force-pushed the update-guide branch 2 times, most recently from 88ef5f7 to e3c8435 Compare January 7, 2022 09:47
@turkenh turkenh merged commit ac613b2 into crossplane:main Jan 7, 2022
@turkenh turkenh deleted the update-guide branch January 7, 2022 09:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update configuring resource guide per latest changes
3 participants