Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

migrate: Ensure idempotency when ran multiple times #364

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Apr 26, 2024

Closes #362

To reproduce, copied an existing test case and ensured the test script included multiple exec tfplugindocs migrate invocations. This generated a test failure matching the bug report:

--- FAIL: Test_SchemaJson_MigrateAcceptanceTests (0.00s)
    --- FAIL: Test_SchemaJson_MigrateAcceptanceTests/time_provider_multiple_runs (0.32s)
        testscript.go:558: # Copyright (c) HashiCorp, Inc.
            # SPDX-License-Identifier: MPL-2.0
            # Multiple runs of tfplugindocs -migrate to verify idempotency (0.000s)
            # Run migrate command (0.301s)
            # Check template files (0.000s)
            > cmpenv templates/index.md.tmpl exp-templates/index.md.tmpl
            diff templates/index.md.tmpl exp-templates/index.md.tmpl
            --- templates/index.md.tmpl
            +++ exp-templates/index.md.tmpl
            @@ -31,36 +31,3 @@
             `triggers` are *not* treated as sensitive attributes; a value used for `triggers` will be displayed in Terraform UI output as plaintext.

             To force a these actions to reoccur without updating `triggers`, the [`terraform taint` command](https://www.terraform.io/docs/commands/taint.html) can be used to produce the action on the next run.
            ----
            -page_title: "Provider: Time"
            -description: |-
            -  The time provider is used to interact with time-based resources.
            ----
            -
            -{{/* This template serves as a starting point for documentation generation, and can be customized with hardcoded values and/or doc gen templates.
            -
            -For example, the {{ .SchemaMarkdown }} template can be used to replace manual schema documentation if descriptions of schema attributes are added in the provider source code. */ -}}
            -
            -# Time Provider
            -
            -The time provider is used to interact with time-based resources. The provider itself has no configuration options.
            -
            -Use the navigation to the left to read about the available resources.
            -
            -## Resource "Triggers"
            -
            -Certain time resources, only perform actions during specific lifecycle actions:
            -
            -- `time_offset`: Saves base timestamp into Terraform state only when created.
            -- `time_sleep`: Sleeps when created and/or destroyed.
            -- `time_static`: Saves base timestamp into Terraform state only when created.
            -
            -These resources provide an optional map argument called `triggers` that can be populated with arbitrary key/value pairs. When the keys or values of this argument are updated, Terraform will re-perform the desired action, such as updating the base timestamp or sleeping again.
            -
            -For example:
            -
            -{{tffile "examples/example_1.tf"}}
            -
            -`triggers` are *not* treated as sensitive attributes; a value used for `triggers` will be displayed in Terraform UI output as plaintext.
            -
            -To force a these actions to reoccur without updating `triggers`, the [`terraform taint` command](https://www.terraform.io/docs/commands/taint.html) can be used to produce the action on the next run.

            FAIL: testdata/scripts/schema-json/migrate/time_provider_multiple_runs.txtar:13: templates/index.md.tmpl and exp-templates/index.md.tmpl differ

To fix this, changed the internal logic from opening the template file multiple times with os.O_APPEND to opening the template file once with only os.O_WRONLY|os.O_CREATE and passing the file handle around instead.

Reference: #362

To reproduce, copied an existing test case and ensured the test script included multiple `exec tfplugindocs migrate` invocations. This generated a test failure matching the bug report:

```
--- FAIL: Test_SchemaJson_MigrateAcceptanceTests (0.00s)
    --- FAIL: Test_SchemaJson_MigrateAcceptanceTests/time_provider_multiple_runs (0.32s)
        testscript.go:558: # Copyright (c) HashiCorp, Inc.
            # SPDX-License-Identifier: MPL-2.0
            # Multiple runs of tfplugindocs -migrate to verify idempotency (0.000s)
            # Run migrate command (0.301s)
            # Check template files (0.000s)
            > cmpenv templates/index.md.tmpl exp-templates/index.md.tmpl
            diff templates/index.md.tmpl exp-templates/index.md.tmpl
            --- templates/index.md.tmpl
            +++ exp-templates/index.md.tmpl
            @@ -31,36 +31,3 @@
             `triggers` are *not* treated as sensitive attributes; a value used for `triggers` will be displayed in Terraform UI output as plaintext.

             To force a these actions to reoccur without updating `triggers`, the [`terraform taint` command](https://www.terraform.io/docs/commands/taint.html) can be used to produce the action on the next run.
            ----
            -page_title: "Provider: Time"
            -description: |-
            -  The time provider is used to interact with time-based resources.
            ----
            -
            -{{/* This template serves as a starting point for documentation generation, and can be customized with hardcoded values and/or doc gen templates.
            -
            -For example, the {{ .SchemaMarkdown }} template can be used to replace manual schema documentation if descriptions of schema attributes are added in the provider source code. */ -}}
            -
            -# Time Provider
            -
            -The time provider is used to interact with time-based resources. The provider itself has no configuration options.
            -
            -Use the navigation to the left to read about the available resources.
            -
            -## Resource "Triggers"
            -
            -Certain time resources, only perform actions during specific lifecycle actions:
            -
            -- `time_offset`: Saves base timestamp into Terraform state only when created.
            -- `time_sleep`: Sleeps when created and/or destroyed.
            -- `time_static`: Saves base timestamp into Terraform state only when created.
            -
            -These resources provide an optional map argument called `triggers` that can be populated with arbitrary key/value pairs. When the keys or values of this argument are updated, Terraform will re-perform the desired action, such as updating the base timestamp or sleeping again.
            -
            -For example:
            -
            -{{tffile "examples/example_1.tf"}}
            -
            -`triggers` are *not* treated as sensitive attributes; a value used for `triggers` will be displayed in Terraform UI output as plaintext.
            -
            -To force a these actions to reoccur without updating `triggers`, the [`terraform taint` command](https://www.terraform.io/docs/commands/taint.html) can be used to produce the action on the next run.

            FAIL: testdata/scripts/schema-json/migrate/time_provider_multiple_runs.txtar:13: templates/index.md.tmpl and exp-templates/index.md.tmpl differ
```

To fix this, changed the internal logic from opening the template file multiple times with `os.O_APPEND` to opening the template file once with only `os.O_WRONLY|os.O_CREATE` and passing the file handle around instead.
@bflad bflad added the bug Something isn't working label Apr 26, 2024
@bflad bflad added this to the v0.19.2 milestone Apr 26, 2024
@bflad bflad requested a review from a team as a code owner April 26, 2024 13:23
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

LGTM, will probably want a changelog as well 🚀

@bflad
Copy link
Contributor Author

bflad commented Apr 26, 2024

Ha whoops I had committed but not pushed that up! Adding now.

@bflad bflad merged commit 8a55c41 into main Apr 26, 2024
6 checks passed
@bflad bflad deleted the bflad/migrate-no-more-append branch April 26, 2024 14:05
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate appends a new copy of the file each time it is executed.
2 participants