-
Notifications
You must be signed in to change notification settings - Fork 46
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
HCPE-988 - Add initial contribution guidelines #71
Conversation
|
||
_Note: This documentation is intended for Terraform HCP Provider code developers. Typical operators writing and applying Terraform configurations do not need to read or understand this material._ | ||
|
||
## Beta Features |
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.
👌
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.
Nice work, really appreciate the thorough detail here!
|
||
The following checklists are meant to be used for PRs to give developers and reviewers confidence that the proper changes have been made: | ||
|
||
* [New resource](checklist-resource.md) |
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.
is it worth adding a new data source
section?
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 don't think so. The aws provider doesn't do this, and I think we can direct developers to the resource checklist, where they can skip the items that do not apply.
- [ ] __Uses Context-Aware CRUD Functions__: The context-aware CRUD functions (eg. `CreateContext`, `ReadContext`, etc.) should be used over their deprecated counterparts (eg. `Create`, `Read`, etc.). | ||
- [ ] __Uses Context For API Calls__: The `context.Context` that is passed into the CRUD functions should be passed into all API calls, most often by setting the `Context` field of a `*Params` object. This allows the API calls to be cancelled properly by Terraform. | ||
- [ ] __Handles Existing Resource Prior To Create__: Before calling the API creation function, there should be a check to ensure that the resource does not already exist. If it does exist, the user should see a helpful log message that they may need to import the resource. | ||
- [ ] __Implements Immediate Resource ID Set During Create__: Immediately after calling the API creation function, the resource ID should be set with [`d.SetId()`](https://godoc.org/github.com/hashicorp/terraform/helper/schema#ResourceData.SetId) before other API operations or returning. |
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.
Add some context why?
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 is actually copy-and-pasted from this section in the aws provider. I think it's in line with the adjacent checks in terms of the level of detail provided, and it's not the type of thing that I would expect to spur debate or confusion if there is not an explicit rationale here.
* Refactor HCP Vault TF acceptance tests for Azure changes remove out.txt fix perf replication broken tests * feedback * feedback
This initial set of guidelines aims to capture some of the design decisions and patterns that have already been made, and provide some context around why they were chosen.
The format is largely stolen from terraform-provider-aws, and to a lesser extent, nomad and terraform-provider-boundary. Checklists appear to be a standard way of communicating the list of implementation items to consider while making a change while providing some context, and the references concept gives us a place to keep background information around broader concepts.