Skip to content

Latest commit

 

History

History
185 lines (144 loc) · 15.7 KB

pullrequest-submission-and-lifecycle.md

File metadata and controls

185 lines (144 loc) · 15.7 KB
Raw

Pull Request Submission and Lifecycle

We appreciate direct contributions to the provider codebase. Here's what to expect:

  • For pull requests that follow the guidelines, we will proceed to reviewing and merging, following the provider team's review schedule. There may be some internal or community discussion needed before we can complete this.
  • Pull requests that don't follow the guidelines will be commented with what they're missing. The person who submits the pull request or another community member will need to address those requests before they move forward.

Pull Request Lifecycle

  1. Fork the GitHub repository, modify the code, and create a pull request. You are welcome to submit your pull request for commentary or review before it is fully completed by creating a draft pull request or adding [WIP] to the beginning of the pull request title. Please include specific questions or items you'd like feedback on.

  2. Once you believe your pull request is ready to be reviewed, ensure the pull request is not a draft pull request by marking it ready for review or removing [WIP] from the pull request title if necessary, and a maintainer will review it. Follow the checklists below to help ensure that your contribution can be easily reviewed and potentially merged.

  3. One of Terraform's provider team members will look over your contribution and either approve it or provide comments letting you know if there is anything left to do. We'll try give you the opportunity to make the required changes yourself, but in some cases we may perform the changes ourselves if it makes sense to (minor changes, or for urgent issues). We do our best to keep up with the volume of PRs waiting for review, but it may take some time depending on the complexity of the work.

  4. Once all outstanding comments and checklist items have been addressed, your contribution will be merged! Merged PRs will be included in the next Terraform release. The provider team takes care of updating the CHANGELOG as they merge.

  5. In some cases, we might decide that a PR should be closed without merging. We'll make sure to provide clear reasoning when this happens.

Branch Prefixes

We try to use a common set of branch name prefixes when submitting pull requests. Prefixes give us an idea of what the branch is for. For example, td-staticcheck-st1008 would let us know the branch was created to fix a technical debt issue, and f-aws_emr_instance_group-refactor would indicate a feature request for the aws_emr_instance_group resource that’s being refactored. These are the prefixes we currently use:

  • f = feature
  • b = bug fix
  • d = documentation
  • t = tests
  • td = technical debt
  • v = dependencies ("vendoring" previously)

Conventions across non-AWS providers varies so when working with other providers please check the names of previously created branches and conform to their standard practices.

Common Review Items

The Terraform AWS Provider follows common practices to ensure consistent and reliable implementations across all resources in the project. While there may be older resource and testing code that predates these guidelines, new submissions are generally expected to adhere to these items to maintain Terraform Provider quality. For any guidelines listed, contributors are encouraged to ask any questions and community reviewers are encouraged to provide review suggestions based on these guidelines to speed up the review and merge process.

Go Coding Style

The following Go language resources provide common coding preferences that may be referenced during review, if not automatically handled by the project's linting tools.

Resource Contribution Guidelines

The following resource checks need to be addressed before your contribution can be merged. The exclusion of any applicable check may result in a delayed time to merge.

  • Passes Testing: All code and documentation changes must pass unit testing, code linting, and website link testing. Resource code changes must pass all acceptance testing for the resource.

  • Avoids API Calls Across Account, Region, and Service Boundaries: Resources should not implement cross-account, cross-region, or cross-service API calls.

  • Avoids Optional and Required for Non-Configurable Attributes: Resource schema definitions for read-only attributes should not include Optional: true or Required: true.

  • Avoids resource.Retry() without resource.RetryableError(): Resource logic should only implement resource.Retry() if there is a retryable condition (e.g. return resource.RetryableError(err)).

  • Avoids Resource Read Function in Data Source Read Function: Data sources should fully implement their own resource Read functionality including duplicating d.Set() calls.

  • Avoids Reading Schema Structure in Resource Code: The resource Schema should not be read in resource Create/Read/Update/Delete functions to perform looping or otherwise complex attribute logic. Use d.Get() and d.Set() directly with individual attributes instead.

  • Avoids ResourceData.GetOkExists(): Resource logic should avoid using ResourceData.GetOkExists() as its expected functionality is not guaranteed in all scenarios.

  • Implements Read After Create and Update: Except where API eventual consistency prohibits immediate reading of resources or updated attributes, resource Create and Update functions should return the resource Read function.

  • Implements Immediate Resource ID Set During Create: Immediately after calling the API creation function, the resource ID should be set with d.SetId() before other API operations or returning the Read function.

  • Implements Attribute Refreshes During Read: All attributes available in the API should have d.Set() called their values in the Terraform state during the Read function.

  • Implements Error Checks with Non-Primitive Attribute Refreshes: When using d.Set() with non-primitive types (schema.TypeList, schema.TypeSet, or schema.TypeMap), perform error checking to prevent issues where the code is not properly able to refresh the Terraform state.

  • Implements Import Acceptance Testing and Documentation: Support for resource import (Importer in resource schema) must include ImportState acceptance testing (see also the Acceptance Testing Guidelines below) and ## Import section in resource documentation.

  • Implements Customizable Timeouts Documentation: Support for customizable timeouts (Timeouts in resource schema) must include ## Timeouts section in resource documentation.

  • Implements State Migration When Adding New Virtual Attribute: For new "virtual" attributes (those only in Terraform and not in the API), the schema should implement State Migration to prevent differences for existing configurations that upgrade.

  • Uses AWS Go SDK Constants: Many AWS services provide string constants for value enumerations, error codes, and status types. See also the "Constants" sections under each of the service packages in the AWS Go SDK documentation.

  • Uses AWS Go SDK Pointer Conversion Functions: Many APIs return pointer types and these functions return the zero value for the type if the pointer is nil. This prevents potential panics from unchecked * pointer dereferences and can eliminate boilerplate nil checking in many cases. See also the aws package in the AWS Go SDK documentation.

  • Uses AWS Go SDK Types: Use available SDK structs instead of implementing custom types with indirection.

  • Uses Existing Validation Functions: Schema definitions including ValidateFunc for attribute validation should use available Terraform helper/validation package functions. All()/Any() can be used for combining multiple validation function behaviors.

  • Uses tfresource.TimedOut() with resource.Retry(): Resource logic implementing resource.Retry() should error check with tfresource.TimedOut(err error) and potentially unset the error before returning the error. For example:

    var output *kms.CreateKeyOutput
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
  var err error

  output, err = conn.CreateKey(input)

  /* ... */

  return nil
})

if tfresource.TimedOut(err) {
  output, err = conn.CreateKey(input)
}

if err != nil {
  return fmt.Errorf("error creating KMS External Key: %s", err)
}

  • Uses resource.NotFoundError: Custom errors for missing resources should use resource.NotFoundError.

  • Uses resource.UniqueId(): API fields for concurrency protection such as CallerReference and IdempotencyToken should use resource.UniqueId(). The implementation includes a monotonic counter which is safer for concurrent operations than solutions such as time.Now().

  • Skips id Attribute: The id attribute is implicit for all Terraform resources and does not need to be defined in the schema.

The below are style-based items that may be noted during review and are recommended for simplicity, consistency, and quality assurance:

  • Avoids CustomizeDiff: Usage of CustomizeDiff is generally discouraged.

  • Implements Error Message Context: Returning errors from resource Create, Read, Update, and Delete functions should include additional messaging about the location or cause of the error for operators and code maintainers by wrapping with fmt.Errorf().

    • An example Delete API error: return fmt.Errorf("error deleting {SERVICE} {THING} (%s): %w", d.Id(), err)
    • An example d.Set() error: return fmt.Errorf("error setting {ATTRIBUTE}: %w", err)

  • Implements arn Attribute: APIs that return an Amazon Resource Name (ARN) should implement arn as an attribute. Alternatively, the ARN can be synthesized using the AWS Go SDK arn.ARN structure. For example:

    // Direct Connect Virtual Interface ARN.
// See https://docs.aws.amazon.com/directconnect/latest/UserGuide/security_iam_service-with-iam.html#security_iam_service-with-iam-id-based-policies-resources.
  arn := arn.ARN{
  	Partition: meta.(*AWSClient).partition,
  	Region:    meta.(*AWSClient).region,
  	Service:   "directconnect",
  	AccountID: meta.(*AWSClient).accountid,
  	Resource:  fmt.Sprintf("dxvif/%s", d.Id()),
  }.String()
  d.Set("arn", arn)

    When the arn attribute is synthesized this way, add the resource to the list of those affected by the provider's skip_requesting_account_id attribute.

  • Implements Warning Logging With Resource State Removal: If a resource is removed outside of Terraform (e.g. via different tool, API, or web UI), d.SetId("") and return nil can be used in the resource Read function to trigger resource recreation. When this occurs, a warning log message should be printed beforehand: log.Printf("[WARN] {SERVICE} {THING} (%s) not found, removing from state", d.Id())

  • Uses Functions from aws-sdk-go-base/tfawserr with AWS Go SDK Error Objects: Use the ErrCodeEquals(err error, code string) and ErrMessageContains(err error, code string, message string) helper functions instead of the awserr package to compare error code and message contents.

  • Uses %s fmt Verb with AWS Go SDK Objects: AWS Go SDK objects implement String() so using the %v, %#v, or %+v fmt verbs with the object are extraneous or provide unhelpful detail.

  • Uses American English for Attribute Naming: For any ambiguity with attribute naming, prefer American English over British English. e.g. color instead of colour.

  • Skips Timestamp Attributes: Generally, creation and modification dates from the API should be omitted from the schema.

  • Skips Error() Call with AWS Go SDK Error Objects: Error objects do not need to have Error() called.

  • Adds Error Codes Missing from the AWS Go SDK to Internal Service Package: If an AWS API error code is checked and the AWS Go SDK has no constant string value defined for that error code, a new constant should be added to a file named errors.go in a per-service internal package. For example:

// In `aws/internal/service/s3/errors.go`.

package s3

const (
	ErrCodeNoSuchTagSet = "NoSuchTagSet"
)
// Example usage.

import (
	"github.com/aws/aws-sdk-go/service/s3"
	"github.com/hashicorp/aws-sdk-go-base/tfawserr"
	tfs3 "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/s3"
)

output, err := conn.GetBucketTagging(input)

if tfawserr.ErrCodeEquals(err, tfs3.ErrCodeNoSuchTagSet) {
	return nil
}
  • Uses Paginated AWS Go SDK Functions When Iterating Over a Collection of Objects: When the API for listing a collection of objects provides a paginated function, use it instead of looping until the next page token is not set. For example, with the EC2 API, DescribeInstancesPages should be used instead of DescribeInstances when more than one result is expected.
  • Adds Paginated Functions Missing from the AWS Go SDK to Internal Service Package: If the AWS Go SDK does not define a paginated equivalent for a function to list a collection of objects, it should be added to a per-service internal package using the listpages generator. A support case should also be opened with AWS to have the paginated functions added to the AWS Go SDK.