Contributing to Terraform - Active Directory Provider

First: if you’re unsure or afraid of anything, ask for help! You can submit a work in progress (WIP) pull request, or file an issue with the parts you know. We’ll do our best to guide you in the right direction, and let you know if there are guidelines we will need to follow. We want people to be able to participate without fear of doing the wrong thing.

Below are our expectations for contributors. Following these guidelines gives us the best opportunity to work with you, by making sure we have the things we need in order to make it happen. Doing your best to follow it will speed up our ability to merge PRs and respond to issues.

• Issues
  • Issue Reporting Checklists
    • Bug Reports
    • Feature Requests
    • Questions
  • Issue Lifecycle
• Pull Requests
  • Pull Request Lifecycle
  • Checklists for Contribution
    • Documentation Update
    • Enhancement/Bugfix to a Resource
    • New Resource
  • Common Review Items
    • Go Coding Style
    • Resource Contribution Guidelines
    • Acceptance Testing Guidelines
  • Writing Acceptance Tests
    • Acceptance Tests Often Cost Money to Run
    • Running an Acceptance Test
    • Writing an Acceptance Test

Issues

Issue Reporting Checklists

We welcome issues of all kinds including feature requests, bug reports, and general questions. Below you’ll find checklists with guidelines for well-formed issues of each type.

^{back to top}

Bug Reports

• Test against latest release: Make sure you test against the latest released version. It is possible we already fixed the bug you’re experiencing.

• Search for possible duplicate reports: It’s helpful to keep bug reports consolidated to one thread, so do a quick search on existing bug reports to check if anybody else has reported the same thing. You can scope searches by the label “bug” to help narrow things down.

• Include steps to reproduce: Provide steps to reproduce the issue, along with your .tf files, with secrets removed, so we can try to reproduce it. Without this, it makes it much harder to fix the issue.

• For panics, include crash.log: If you experienced a panic, please create a gist of the entire generated crash log for us to look at. Double check no sensitive items were in the log.

^{back to top}

Feature Requests

• Search for possible duplicate requests: It’s helpful to keep requests consolidated to one thread, so do a quick search on existing requests to check if anybody else has reported the same thing. You can scope searches by the label “enhancement” to help narrow things down.

• Include a use case description: In addition to describing the behavior of the feature you’d like to see added, it’s helpful to also lay out the reason why the feature would be important and how it would benefit Terraform users.

^{back to top}

Questions

• Search for answers in Terraform documentation: We’re happy to answer questions in GitHub Issues. Oftentimes Question issues result in documentation updates to help future users, so if you don’t find an answer, you can give us pointers for where you’d expect to see it in the docs.

^{back to top}

Issue Lifecycle

1. The issue is reported.

2. The issue is verified and categorized. Categorization is done via GitHub labels.

3. An initial process determines whether the issue is critical and must be addressed immediately, or can be left open for community discussion.

4. The issue is addressed in a pull request or commit. The issue number will be referenced in the commit message so that the code that fixes it is clearly linked.

5. The issue is closed. Sometimes, valid issues will be closed because they are tracked elsewhere or non-actionable. The issue is still indexed and available for future viewers, or can be re-opened if necessary.

^{back to top}

Pull Requests

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.

^{back to top}

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 team members will look over your contribution and either approve it or provide comments letting you know if there is anything left to do.

4. Once all outstanding comments and checklist items have been addressed, your contribution will be merged! Merged PRs will be included in the next 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.

^{back to top}

Checklists for Contribution

There are several different kinds of contribution, each of which has its own standards for a speedy review. The following sections describe guidelines for each type of contribution.

^{back to top}

Documentation Update (WIP)

^{back to top}

Enhancement/Bugfix to a Resource

Working on existing resources is a great way to get started as a Terraform contributor because you can work within existing code and tests to get a feel for what to do.

In addition to the below checklist, please see the Common Review Items sections for more specific coding and testing guidelines.

• Acceptance test coverage of new behavior: Existing resources each have a set of acceptance tests covering their functionality. These tests should exercise all the behavior of the resource. Whether you are adding something or fixing a bug, the idea is to have an acceptance test that fails if your code were to be removed. Sometimes it is sufficient to “enhance” an existing test by adding an assertion or tweaking the config that is used, but it’s often better to add a new test. You can copy/paste an existing test and follow the conventions you see there, modifying the test to exercise the behavior of your code.
• Documentation updates (WIP): If your code makes any changes that need to be documented, you should include those doc updates in the same PR. This includes things like new resource attributes or changes in default values. The Terraform website source is in this repo and includes instructions for getting a local copy of the site up and running if you’d like to preview your changes.
• Well-formed Code: Do your best to follow existing conventions you see in the codebase, and ensure your code is formatted with go fmt. (The Circle CI build will fail if go fmt has not been run on incoming code.) The PR reviewers can help out on this front, and may provide comments with suggestions on how to improve the code.
• Vendor additions: Create a separate PR if you are updating the vendor folder. This is to avoid conflicts as the vendor versions tend to be fast- moving targets. We will plan to merge the PR with this change first.

^{back to top}

New Resource

Implementing a new resource is a good way to learn more about how Terraform interacts with upstream APIs. There are plenty of examples to draw from in the existing resources, but you still get to implement something completely new.

In addition to the below checklist, please see the Common Review Items sections for more specific coding and testing guidelines.

• Minimal LOC: It’s difficult for both the reviewer and author to go through long feedback cycles on a big PR with many resources. We ask you to only submit 1 resource at a time.
• Acceptance tests: New resources should include acceptance tests covering their behavior. See Writing Acceptance Tests below for a detailed guide on how to approach these.
• Resource Naming: Resources should be named activedirectory_<name>, using underscores (_) as the separator.
• Arguments_and_Attributes: The HCL for arguments and attributes should mimic the types and structs presented by the Active Directory API. API arguments should be converted from CamelCase to camel_case.
• Documentation: - need to be done
• Well-formed Code: Do your best to follow existing conventions you see in the codebase, and ensure your code is formatted with go fmt. (The Travis CI build will fail if go fmt has not been run on incoming code.) The PR reviewers can help out on this front, and may provide comments with suggestions on how to improve the code.
• Vendor updates: Create a separate PR if you are adding to the vendor folder. This is to avoid conflicts as the vendor versions tend to be fast- moving targets. We will plan to merge the PR with this change first.

^{back to top}

Common Review Items

The Terraform Active Directory 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.

^{back to top}

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.

• Effective Go
• Go Code Review Comments

^{back to top}

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 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-Primative Attribute Refreshes: When using d.Set() with non-primative 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 (WIP):
• 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 TypeList and MaxItems: 1: Configuration block attributes (e.g. Type: schema.TypeList or Type: schema.TypeSet with Elem: &schema.Resource{...}) that can only have one block should use Type: schema.TypeList and MaxItems: 1 in the schema definition.
• 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 resource.NotFoundError: Custom errors for missing resources should use resource.NotFoundError.
• Skips Exists Function: Implementing a resource Exists function is extraneous as it often duplicates resource Read functionality. Ensure d.SetId("") is used to appropriately trigger resource recreation in the resource Read function.
• 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 (WIP): 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 {THING} (%s): %s", d.Id(), err)
  • An example d.Set() error: return fmt.Errorf("error setting {ATTRIBUTE}: %s", err)
• Implements Warning Logging With Resource State Removal (WIP): 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] {THING} (%s) not found, removing from state", d.Id())
• Uses Elem with TypeMap: While provider schema validation does not error when the Elem configuration is not present with Type: schema.TypeMap attributes, including the explicit Elem: &schema.Schema{Type: schema.TypeString} is recommended.
• 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: Error objects do not need to have Error() called.

^{back to top}

Acceptance Testing Guidelines

The below are required items that will be noted during submission review and prevent immediate merging:

• Implements CheckDestroy: Resource testing should include a CheckDestroy function (typically named testAccCheckAD{RESOURCE}Destroy) that calls the API to verify that the Terraform resource has been deleted or disassociated as appropriate. More information about CheckDestroy functions can be found in the Extending Terraform TestCase documentation.
• Implements Exists Check Function: Resource testing should include a TestCheckFunc function (typically named testAccCheckAD{RESOURCE}Exists) that calls the API to verify that the Terraform resource has been created or associated as appropriate. Preferably, this function will also accept a pointer to an API object representing the Terraform resource from the API response that can be set for potential usage in later TestCheckFunc. More information about these functions can be found in the Extending Terraform Custom Check Functions documentation.
• Excludes Provider Declarations: Test configurations should not include provider "activedirectory" {...} declarations. If necessary, only the provider declarations in provider_test.go should be used for multiple account/region or otherwise specialized testing.
• Uses resource.ParallelTest (WIP): Tests should utilize resource.ParallelTest() instead of resource.Test() except where serialized testing is absolutely required.
• Uses fmt.Sprintf(): Test configurations preferably should to be separated into their own functions (typically named testAccADRESOURCE}Config{PURPOSE}) that call fmt.Sprintf() for variable injection or a string const for completely static configurations. Test configurations should avoid var or other variable injection functionality such as text/template.
• Uses Randomized Infrastructure Naming (WIP): Test configurations that utilize resources where a unique name is required should generate a random name. Typically this is created via rName := acctest.RandomWithPrefix("tf-acc-test") in the acceptance test function before generating the configuration.

^{back to top}

Writing Acceptance Tests

Terraform includes an acceptance test harness that does most of the repetitive work involved in testing a resource. For additional information about testing Terraform Providers, see the Extending Terraform documentation.

^{back to top}

Acceptance Tests Often Cost Money to Run

Because acceptance tests create real resources, they often cost money to run. Because the resources only exist for a short period of time, the total amount of money required is usually a relatively small.

^{back to top}

Running an Acceptance Test

Acceptance tests can be run using the testacc target in the Terraform Makefile. The individual tests to run can be controlled using a regular expression. Prior to running the tests provider configuration details such as access keys must be made available as environment variables.

For example, to run an acceptance test against the Active Directory provider, the following environment variables must be set:

export AD_HOST=...
export AD_BIND_USER=...
export AD_BIND_PASSWORD=...
export AD_COMPUTER_TEST_BASE_OU=...

^{back to top}

Writing an Acceptance Test

Terraform has a framework for writing acceptance tests which minimises the amount of boilerplate code necessary to use common testing patterns. The entry point to the framework is the resource.ParallelTest() function.

Tests are divided into TestSteps. Each TestStep proceeds by applying some Terraform configuration using the provider under test, and then verifying that results are as expected by making assertions using the provider API. It is common for a single test function to exercise both the creation of and updates to a single resource. Most tests follow a similar structure.

1. Pre-flight checks are made to ensure that sufficient provider configuration is available to be able to proceed - for example in an acceptance test targeting AD, AD_HOST, AD_BIND_USER, AD_BIND_PASSWORD and AD_COMPUTER_TEST_BASE_OU must be set prior to running acceptance tests. This is common to all tests exercising a single provider.

Each TestStep is defined in the call to resource.ParallelTest(). Most assertion functions are defined out of band with the tests. This keeps the tests readable, and allows reuse of assertion functions across different tests of the same type of resource. The definition of a complete test looks like this:

func TestAccADComputer_basic(t *testing.T) {
	ou := os.Getenv("AD_COMPUTER_TEST_BASE_OU")
	name := "test-acc-computer"
	description := "terraform"

	var computer Computer

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckADComputerDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccResourceADComputerTestData(ou, name, description),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckADComputerExists("activedirectory_computer.test", &computer),
					testAccCheckADComputerAttributes(&computer, ou, name, description),
					resource.TestCheckResourceAttr("activedirectory_computer.test", "ou", ou),
					resource.TestCheckResourceAttr("activedirectory_computer.test", "name", name),
					resource.TestCheckResourceAttr("activedirectory_computer.test", "description", description),
					resource.TestCheckResourceAttr("activedirectory_computer.test", "id", fmt.Sprintf("cn=%s,%s", name, ou)),
				),
			},
		},
	})
}

^{back to top}