Skip to content
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

Update Contributor Docs and Resource-Specific Import Examples to Reflect Multi-Project Support #517

Merged
merged 9 commits into from Jun 7, 2023

Conversation

aidan-mundy
Copy link
Contributor

🛠️ Description

Updates contributor documentation and resource-specific import examples to reflect multi-project support added by #454

The new resource and resource import checklists previously prohibited project_id arguments. This PR updates the checklists to require optional project_id attributes (for resources) and import ID prefixes (for importing) on all resources that have multi-project support.

Resources with multi-project support did not have documented examples for importing with non-default project IDs, this PR adds those examples.

🏗️ Acceptance tests

N/A, documentation only change

@aidan-mundy aidan-mundy added the documentation Improvements or additions to documentation label May 30, 2023
@hashicorp-cla
Copy link

hashicorp-cla commented May 30, 2023

CLA assistant check
All committers have signed the CLA.

@aidan-mundy aidan-mundy marked this pull request as ready for review May 30, 2023 19:27
@aidan-mundy aidan-mundy requested a review from a team as a code owner May 30, 2023 19:27
@aidan-mundy aidan-mundy requested a review from a team May 30, 2023 19:27
@aidan-mundy aidan-mundy requested review from a team as code owners May 30, 2023 19:27
Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is off to a good start. The docs are generated using the contents of the example directory so these changes need to be made there.

Comment on lines +73 to +77
# Using an explicit project ID, the import ID is:
# {project_id}:{hvn_id}
terraform import hcp_hvn.example f709ec73-55d4-46d8-897d-816ebba28778:main-hvn
# Using the provider-default project ID, the import ID is:
# {hvn_id}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I believe these changes need to be added into the respective import.sh file within the resource example docs https://github.com/hashicorp/terraform-provider-hcp/tree/main/examples/resources/hcp_hvn.

These files are auto generated using templates.

Instead of relying on comments with the code block. What about creating a separate code block for the multi-project example

For example

Import

Import is supported using the following syntax:

Using an explicit project ID, the import ID is:

# {project_id}:{hvn_id}
terraform import hcp_hvn.example f709ec73-55d4-46d8-897d-816ebba28778:main-hvn

Using the provider-default project ID:

# The import ID is {hvn_id}
terraform import hcp_hvn.example main-hvn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to the .md files come from running make gencheck which regenerates the docs directory. The source for the changes in this PR are the import.sh files as you noted. I think you may have missed the edits to the .sh files because the sort order places them below the .md files in the GitHub file browser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was avoiding creating resource-specific templates if there are other clear ways of representing the information because AFAIK it prevents resources from being automatically adjusted if/when the default template changes.

I'm not sure if there actually is a scenario where we would update the default template. If there isn't one, then there's no harm in creating custom templates here.

Edit: An example of custom templates leading to drift from the default: packer_image_iteration and packer_image data sources are slightly out of sync with the format used by other data sources. Specifically, the page_title field is in a different order. (This is also the case for many of the resources with custom templates.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.. thanks for calling this out. I realize now that when I clicked into the import.sh file I was looking at the one on main and not the import.sh associated with this branch.

I understand your point about using custom templates I was under the impression that each of the resources already had a custom template. But I see that is not true.

This is a nit for sure but not one worth rocking the boat for if the import details work for folks

image

Copy link
Member

@nywilken nywilken May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively you can do something like the following to reduce the context of the code block

# {project_id}:{hvn_id}
terraform import hcp_hvn.example f709ec73-55d4-46d8-897d-816ebba28778:main-hvn
# {project_id} not required when setting a provider-default project ID:
terraform import hcp_hvn.example main-hvn

image (1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file requires manual updating. This is because descriptions are not supported for nested objects in the docs generator. This is the case at least with SDKv2, but I think it is possible with Framework. See here for more

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah thanks for this. At some point I think we might need to deprecate this data source as it has been replace by the hcp_packer_image and hcp_packer_iteration data sources.

Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done. This looks good to me. I left a nit on the import section. I find the comments to be a bit confusing but I just might need to get use to it.

Comment on lines +73 to +77
# Using an explicit project ID, the import ID is:
# {project_id}:{hvn_id}
terraform import hcp_hvn.example f709ec73-55d4-46d8-897d-816ebba28778:main-hvn
# Using the provider-default project ID, the import ID is:
# {hvn_id}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.. thanks for calling this out. I realize now that when I clicked into the import.sh file I was looking at the one on main and not the import.sh associated with this branch.

I understand your point about using custom templates I was under the impression that each of the resources already had a custom template. But I see that is not true.

This is a nit for sure but not one worth rocking the boat for if the import details work for folks

image

Comment on lines +73 to +77
# Using an explicit project ID, the import ID is:
# {project_id}:{hvn_id}
terraform import hcp_hvn.example f709ec73-55d4-46d8-897d-816ebba28778:main-hvn
# Using the provider-default project ID, the import ID is:
# {hvn_id}
Copy link
Member

@nywilken nywilken May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively you can do something like the following to reduce the context of the code block

# {project_id}:{hvn_id}
terraform import hcp_hvn.example f709ec73-55d4-46d8-897d-816ebba28778:main-hvn
# {project_id} not required when setting a provider-default project ID:
terraform import hcp_hvn.example main-hvn

image (1)

@nywilken
Copy link
Member

pinging @hashicorp/vault-cloud and @hashicorp/boundary-cloud for review on this as well.

@blueberry-pi blueberry-pi requested review from himran92 and removed request for helenfufu June 5, 2023 19:56
@aidan-mundy aidan-mundy requested a review from aoripov June 6, 2023 15:13
Copy link
Contributor

@aoripov aoripov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I'm approving on behalf @hashicorp/vault-cloud

Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@delores-hashicorp delores-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

delores-hashicorp

This comment was marked as duplicate.

@uruemu
Copy link
Contributor

uruemu commented Jun 7, 2023

Thanks for doing this.

@aidan-mundy aidan-mundy merged commit b097e57 into main Jun 7, 2023
6 checks passed
@aidan-mundy aidan-mundy deleted the aidan-mundy/multiproject-docs branch June 7, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants