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 resources to be in line with current patterns #82

Merged
merged 5 commits into from
Apr 1, 2021
Merged

Conversation

roaks3
Copy link
Contributor

@roaks3 roaks3 commented Mar 23, 2021

These changes will probably be easiest to review one commit at a time. The updates largely come from items we ran into while working on the TGW resource, but did not yet backport to the other resources:

  • fix: Ensure context is being passed for all HCP API calls - 27be4da
  • improvement: Update comments, docs, and messages to use correct capitalization for network peering - e801537
  • improvement: Log import ID used when an import fails due to parsing - 8204243
  • improvement: Add comment to clarify that Links can be sent in API requests - 45e0370
  • improvement: Update peering wait function to use helper - 86ad652

@roaks3 roaks3 requested a review from a team March 23, 2021 20:42
@roaks3 roaks3 changed the title Update resources to be in-line with current patterns Update resources to be in line with current patterns Mar 24, 2021
Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

lookin tidy! 🧹 ✨

@@ -22,7 +22,7 @@ var peeringDeleteTimeout = time.Minute * 35

func resourceAwsNetworkPeering() *schema.Resource {
return &schema.Resource{
Description: "The AWS Network peering resource allows you to manage a Network peering between an HVN and a peer AWS VPC.",
Description: "The AWS Network Peering resource allows you to manage a network peering between an HVN and a peer AWS VPC.",
Copy link
Contributor

Choose a reason for hiding this comment

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

The first Network Peering should be lowercased too. Same with the data source (and import maybe?).
I also noticed this in the Transit Gateway Attachment docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yea, this actually came up before for the transit gateway resources and I got the following guidance from Josh: #58 (comment)

In short, because there is a leading AWS, the entire noun gets title-cased. I'm not sure if the guidance has changed, but if not I think this is correct as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh I see 🤔 We might need to fix up some of the consul resources then, they seemed to stick with sentence case even in the descriptions (https://github.com/hashicorp/terraform-provider-hcp/blob/main/internal/provider/resource_consul_cluster.go#L57).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh boy, we've definitely found some edge cases with these 😄

@joshklekamp can you verify that we still have this right? The following line that @bcmdarroch linked seems like it would be incorrect, and we would want to capitalize the second cluster:

The Consul cluster resource allows you to manage an HCP Consul cluster.

Choose a reason for hiding this comment

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

I don't believe we capitalize cluster, only branded features. I'm not familiar with the term "AWS Network Peering resource" though. If it's a feature of AWS's, I say keep it capitalized, but if not, we might want to revise to "AWS network peering resource".

Copy link
Contributor

@bcmdarroch bcmdarroch Mar 30, 2021

Choose a reason for hiding this comment

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

According to this comment:

  • Top-level descriptions are all capitalized: AWS Transit Gateway Attachment, AWS Network Peering, Consul Cluster ( only TGW does this currently, in the first paragraph)
  • Everywhere else is sentence-cased: AWS transit gateway attachment, AWS network peering, Consul cluster

We can definitely go forward with that, and I'll add a ticket for getting consistency across the resources.
But this does have me wondering if the top-level description exception is worth having, since it just ends up in the first paragraph, not a heading or title. 🤔

Also to throw in another wrench, it looks like AWS capitalizes Transit Gateways and lower-cases peering. So do we add a rule that respects the casing of the brand for certain resources?

Choose a reason for hiding this comment

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

It might be helpful to pull in Kaitlin here. I tend to differ to her since design tries to align to the Learn team's writing style. I think the examples you provided @bcmdarroch might need a little revision. I think design's POV would be that:

  • Product-specific features are all capitalized: AWS Transit Gateway Attachment, AWS Peering Connection
  • Everywhere else is sentence-cased: transit gateway attachment, network peering, , peering connection, Consul cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshklekamp thanks for checking in here! What you have described in your last comment is essentially what we do now, but I think we are just off on how to implement the guidance. It sounds like we want to use AWS Transit Gateway Attachment capitalization when we are referring to the AWS-specific feature by name, but we actually don't do that anywhere in these docs. In the top-level descriptions, we are referring to our HCP Terraform resource by name (ie. a human-readable version of aws_transit_gateway_attachment).

I'm not familiar with the term "AWS Network Peering resource" though. If it's a feature of AWS's, I say keep it capitalized, but if not, we might want to revise to "AWS network peering resource".

So it sounds like we would want The AWS network peering resource for peering, and The AWS transit gateway attachment resource for TGW. I can just update both resources in this PR.

@roaks3 roaks3 merged commit eede3c1 into main Apr 1, 2021
@roaks3 roaks3 deleted the general-chores branch April 1, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants