Skip to content
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

improvement: Only compare resource changes #426

Merged
merged 1 commit into from
Mar 3, 2025
Merged

Conversation

rdhar
Copy link
Member

@rdhar rdhar commented Feb 27, 2025

Hey @bdalpe, thanks for sharing your fork!

This looks like an excellent suggestion for plan-parity feature: how've you been find this in your workflow?

Do you happen to leverage merge_group event trigger as well?

@bdalpe
Copy link
Contributor

bdalpe commented Feb 28, 2025

Hi @rdhar, thanks for opening the PR.

I was struggling to get the plan-parity flag to work in my environment, hence the fork.

Some background:

  • The Terraform is managing a Kubernetes cluster and resources inside it.
  • Renovate bot manages dependencies and opens automated PRs to update resources like Helm Chart versions, image tags, etc.
  • On PR open, terraform plan is run.
  • On push to the main branch, terraform apply is run.

My action workflow looks something like this:

on:
  pull_request:
    paths:
      - k8s/**/*.tf
  push:
    paths:
      - k8s/**/*.tf
    branches:
      - main

concurrency:
  group: ${{ github.event_name == 'push' && 'apply' || github.ref }}
  cancel-in-progress: false

jobs:
  apply:
    name: Terraform ${{ github.event_name == 'push' && 'Apply' || 'Plan' }}
    runs-on: ubuntu-latest
    timeout-minutes: 10

    permissions:
      id-token: write
      actions: read # Required to download repository artifact.
      checks: write # Required to add status summary.
      contents: read # Required to check out repository.
      pull-requests: write # Required to add PR comment and label.

    steps:
      - name: Checkout
        uses: actions/checkout@v4

      - name: Setup TF
        uses: opentofu/setup-opentofu@v1

        ... some intermediate steps omitted ...

      - name: Terraform ${{ github.event_name == 'push' && 'Apply' || 'Plan' }}
        uses: OP5dev/tf-via-pr@v13
        env:
          ...
        with:
          command: ${{ github.event_name == 'push' && 'apply' || 'plan' }}
          tool: tofu
          arg-lock: ${{ github.event_name == 'push' && 'true' || 'false' }}
          plan-parity: 'true'

After an apply, you have to re-run plan in order for the plan to be in sync with the cluster, otherwise the apply fails. With the existing plan-parity flag, the entire plan is diffed which causes issues.

Kubernetes resources in Terraform track state of the resource_version which is a monotonically increasing integer. It does not trigger a change to any resource, but it does cause the parity diff to fail because the values are different.

My proposal is to use the resource_changes field in the Terraform JSON output format to check which resources are changing and ignore any no-op changes.

With this change, I can click the merge button on multiple PRs and GitHub actions is able to run all of the apply commands in sequence without issue. 😃

Note: I have not tested this with a merge_group workflow.

@rdhar
Copy link
Member Author

rdhar commented Feb 28, 2025

Thanks so much for such a detailed response, this is exactly what I'm looking to learn from other users!

And while I'd intended plan-parity to be only used in Merge Queues for a sequence of merges, I've heard more and more folks use plan-parity in their usual, normal merge-commit workflows so they can benefit from merging multiple PRs in quick succession — just like yourself.

@rdhar rdhar merged commit 9763e7d into OP5dev:main Mar 3, 2025
10 checks passed
@rdhar rdhar mentioned this pull request Mar 3, 2025
@rdhar
Copy link
Member Author

rdhar commented Mar 3, 2025

Happy to share this has been shipped with v13.1.5, where your contribution has been credited!

Please consider ⭐ this project, if you or your team find it useful.

GitHub repository stargazers


@bdalpe Thank YOU for raising this fork proposal — not only should it reduce flaky comparison, it should help speed up execution time as well!

@bdalpe
Copy link
Contributor

bdalpe commented Mar 3, 2025

Thank you for merging!

I'll add one additional item of context for the conversation... I've noticed that with plan-parity: true, that I also need to set refresh: false on apply. Otherwise, I still get some false positives on the diff for things like resource version for those affected resources.

@rdhar
Copy link
Member Author

rdhar commented Mar 3, 2025

When I added plan-parity, I didn't actually expect folks to use the option since it's so much slower to re-run plan before every apply, so I'm glad to see folks appreciate its value in the context of merging multiple PRs in a row!

However, I wish Terraform/Tofu exposed a more resilient plan-file comparison method. Diff-ing massive JSON files is hardly reliable :/

Thanks once again to your input: I've updated the workflow example featuring plan-parity: true to also include conditional arg-refresh: as best practice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants