Skip to content

Latest commit

 

History

History
310 lines (227 loc) · 19.8 KB

pullrequest-submission-and-lifecycle.md

File metadata and controls

310 lines (227 loc) · 19.8 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. Create a changelog entry following the process outlined here

  3. 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.

  4. 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.

  5. 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.

  6. 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

All Go code is automatically checked for compliance with various linters, such as gofmt. These tools can be installed using the GNUMakefile in this repository.

% cd terraform-provider-aws
% make tools

Check your code with the linters:

% make lint

gofmt will also fix many simple formatting issues for you. The Makefile includes a target for this:

% make fmt

The import statement in a Go file follows these rules (see #15903):

  1. Import declarations are grouped into a maximum of three groups with the following order:
    • Standard packages (also called short import path or built-in packages)
    • Third-party packages (also called long import path packages)
    • Local packages
  2. Groups are separated by a single blank line
  3. Packages within each group are alphabetized

Check your imports:

% make importlint

For greater detail, 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. Some of these are not handled by the automated code testing that occurs during submission, so reviewers (even those outside the maintainers) are encouraged to reach out to contributors about any issues to save time.

This Contribution Guide also includes separate sections on topics such as Error Handling, which also applies to contributions.

  • 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.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 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 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.

  • 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.

Changelog Process

HashiCorp’s open-source projects have always maintained user-friendly, readable CHANGELOGs that allow users to tell at a glance whether a release should have any effect on them, and to gauge the risk of an upgrade.

We use the go-changelog to generate and update the changelog from files created in the .changelog/ directory. It is important that when you raise your Pull Request, there is a changelog entry which describes the changes your contribution makes. Not all changes require an entry in the CHANGELOG, guidance follows on what changes do.

Changelog Format

The changelog format requires an entry in the following format, where HEADER corresponds to the changelog category, and the entry is the changelog entry itself. The entry should be included in a file in the .changelog directory with the naming convention {PR-NUMBER}.txt. For example, to create a changelog entry for pull request 1234, there should be a file named .changelog/1234.txt.

```release-note:{HEADER}
{ENTRY}
```

If a pull request should contain multiple changelog entries, then multiple blocks can be added to the same changelog file. For example:

```release-notes:note
resource/aws_example_thing: The `broken` attribute has been deprecated. All configurations using `broken` should be updated to use the new `not_broken` attribute instead.
```

```release-notes:enhancement
resource/aws_example_thing: Add `not_broken` attribute
```

Pull Request Types to CHANGELOG

The CHANGELOG is intended to show operator-impacting changes to the codebase for a particular version. If every change or commit to the code resulted in an entry, the CHANGELOG would become less useful for operators. The lists below are general guidelines and examples for when a decision needs to be made to decide whether a change should have an entry.

Changes that should have a CHANGELOG entry

New resource

A new resource entry should only contain the name of the resource, and use the release-note:new-resource header.

```release-note:new-resource
aws_secretsmanager_secret_policy
```
New data source

A new datasource entry should only contain the name of the datasource, and use the release-note:new-data-source header.

```release-note:new-data-source
aws_workspaces_workspace
```
New full-length documentation guides (e.g. EKS Getting Started Guide, IAM Policy Documents with Terraform)

A new full length documentation entry gives the title of the documentation added, using the release-note:new-guide header.

```release-note:new-guide
Custom Service Endpoint Configuration
```
Resource and provider bug fixes

A new bug entry should use the release-note:bug header and have a prefix indicating the resource or datasource it corresponds to, a colon, then followed by a brief summary. Use a provider prefix for provider level fixes.

```release-note:bug
resource/aws_glue_classifier: Fix quote_symbol being optional
```
Resource and provider enhancements

A new enhancement entry should use the release-note:enhancement header and have a prefix indicating the resource or datasource it corresponds to, a colon, then followed by a brief summary. Use a provider prefix for provider level enchancements.

```release-note:enhancement
resource/aws_eip: Add network_border_group argument
```
Deprecations

A breaking-change entry should use the release-note:note header and have a prefix indicating the resource or datasource it corresponds to, a colon, then followed by a brief summary. Use a provider prefix for provider level changes.

```release-note:note
resource/aws_dx_gateway_association: The vpn_gateway_id attribute is being deprecated in favor of the new associated_gateway_id attribute to support transit gateway associations
```
Breaking Changes and Removals

A breaking-change entry should use the release-note:breaking-change header and have a prefix indicating the resource or datasource it corresponds to, a colon, then followed by a brief summary. Use a provider prefix for provider level changes.

```release-note:breaking-change
resource/aws_lambda_alias: Resource import no longer converts Lambda Function name to ARN
```

Changes that may have a CHANGELOG entry

Dependency updates: If the update contains relevant bug fixes or enhancements that affect operators, those should be called out. Any changes which do not fit into the above categories but warrant highlighting. Use resource/datasource/provider prefixes where appropriate.

```release-note:note
resource/aws_lambda_alias: Resource import no longer converts Lambda Function name to ARN
```

Changes that should not have a CHANGELOG entry

  • Resource and provider documentation updates
  • Testing updates
  • Code refactoring