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.

Sign up for GitHub

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

chore: Runs tests for latest minor versions of each major TF version #1769

Merged
merged 10 commits into from
Dec 19, 2023
Merged

chore: Runs tests for latest minor versions of each major TF version #1769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
71c50d4
concurrency group
lantoli Dec 18, 2023
8a08597
add terraform version to asdf
lantoli Dec 18, 2023
74312c2
TEMPORARY - show terraform version
lantoli Dec 18, 2023
54e8cc2
Revert "TEMPORARY - show terraform version"
lantoli Dec 18, 2023
0d652da
define versions to run
lantoli Dec 18, 2023
6c8960c
versions job
lantoli Dec 18, 2023
4181479
typo
lantoli Dec 18, 2023
5f82991
TEMPORARY - fail linter
lantoli Dec 19, 2023
bd72424
Revert "TEMPORARY - fail linter"
lantoli Dec 19, 2023
d6395ed
Update .github/workflows/test-suite.yml
lantoli Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions .github/workflows/test-suite.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,47 @@ name: 'Test Suite'

on:
workflow_dispatch:
inputs:
terraform_matrix:
description: 'Terraform version matrix (JSON array)'
default: '["1.6.x", "1.5.x", "1.4.x", "1.3.x", "1.2.x"]'
Copy link
Member Author

Choose a reason for hiding this comment

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

I checked that this notation works, e.g. 1.4.x will run version 1.4.7.
TF version list is here: https://releases.hashicorp.com/terraform/

provider_matrix:
description: 'MongoDB Atlas Provider version matrix for running migration tests (JSON array)'
default: '["", "1.12.3"]' # "" for latest version
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we still have 1.12.3 hardcoded here? I think what we want ultimately is that our latest version is compatible... right @Zuhairahmed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@marcosuma Zuhair told me to check our latest 2 minor patch versions, "" is the latest one (currently 1.13.1).
for example once we release 1.14.0, we'll have to change this to: "", "1.13.1", where "" will be 1.14.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why 😅 and also this is something we'll have to manually update too often, every time we make a release (yet one more thing to do manually). Unless there is a strong reason (let's wait for Zuhair) that I am missing let's please remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feedback is to test last 2 Majors of Terraform Atlas Provider versions (BigMajor.Major.Minor format) so today the latest minor patch on each would be v1.13.1 and v1.12.3. This is to get ahead of any unintended breaking changes before our customers were to identify them so we can do this by ensuring the same tests pass last 2 Major versions. Would however explore ways to prevent hardcoding here if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it just me or I am really not seeing the value in this?

Once you've gone over 1.12.3, what is the point in keep testing the second last version? That will never change because we will never touch it.

To me it's more the extra manual effort we are introducing in having to edit the last version every time than the benefit of this

@lantoli @Zuhairahmed

Copy link
Collaborator

Choose a reason for hiding this comment

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

would consider the HashiCorp Terraform testing scenarios where:

  1. acceptance tests fail on Terraform Atlas Provider v1.13.1 and pass on v1.12.3
  2. acceptance tests fail on both Terraform Atlas Provider v1.13.1 and v1.12.3

if we only tested most recent version all we would know for certain is that tests have failed on v1.13.1 and we need to investigate why this occurred which can be from multiple sources. however by running testing on most 2 recent recent version of Terraform Atlas Provider we help accelerate triage of finding a bug by narrowing scope of issue. if scenario 1 one occurs, then we know that error is something that we introduced with most recent changes catching breaking changes ahead of customers. if it's scenario 2, then we know that issue most likely has to do with change made on HashiCorp Terraform side (for example they released bug) so we can then escalate to HashiCorp thereby saving time with bug resolution.

Does this help? Agree again that we should be automating/avoiding hardcodes as much of this as possible.

Copy link
Collaborator

@marcosuma marcosuma Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

removing previous comment

Ok, I got some updates from Leo offline, so I think the main misunderstanding here is that these are migration tests, hence 1.12.3 is used to first create the resource and then "latest" is used to check if there are no plan changes.

Let us discuss this offline.


schedule:
- cron: "0 0 * * *" # workflow runs every day at midnight UTC
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we do longer frequency? every month?

Copy link
Member Author

@lantoli lantoli Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this frequency is for:
- schedule_terraform_matrix: '["1.6.x", "1.0.8"]'
- schedule_provider_matrix: '["", "1.12.3]' # "" for latest version

the longer version test has to be run manually, for instance we can do it monthly or before a release or a major release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we look at the requirement we discussed yesterday, we want to test our latest version against the terraform versions that we declare we support. This was also stated in one of the document Zuhair has recently proposed.

So my ask is: can we please have a monthly-based testing that checks our latest provider version against '["1.6.x", "1.5.x", "1.4.x", "1.3.x", "1.2.x"]'. I'd prefer not to do this manually as we have already lots of things we do manually.

Let me know if I am missing something

Copy link
Member Author

@lantoli lantoli Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we could but the problem I see is that somebody has to look at the results, and if it's done once per month I see it very easy to miss that execution result. Also I find it interesting to run it at other occasions like before a release.

this is also happening with the daily Test Suite. We run them automatically every night, but you have to go to the GH UI to see the results.

If we run something monthly I think that execution will probably be missed.
I don't see the benefit of running it monthly automatically. I think it's better something like I did today: I run them manually and then I look at the results.

Copy link
Member Author

Choose a reason for hiding this comment

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

that said, I you still think it's a good idea please confirm and I'll add the monthly schedule

Copy link
Collaborator

Choose a reason for hiding this comment

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

On this, I don't have a strong opinion, I am ok to run it manually or before a release. @gssbzn how do we do it for AtlasCLI?

Copy link
Collaborator

Choose a reason for hiding this comment

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

how do we do it for AtlasCLI?

We always run packaging on master (we call it the waterfall on evergreen), this means after every merge, and then all the OS compatibility depends on packaging succeeding. The compatibility matrix exploded on the CLI at some point and evergreen let's us generate test programmatically so we write the yaml via code here



concurrency:
group: '${{ github.workflow }}'
cancel-in-progress: false
Copy link
Member Author

Choose a reason for hiding this comment

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

only one Test Suite run at a time to avoid interference. new runs will wait until previous ones finish


jobs:
versions:
env:
schedule_terraform_matrix: '["1.6.x", "1.0.8"]'
schedule_provider_matrix: '["", "1.12.3]' # "" for latest version
Copy link
Member Author

@lantoli lantoli Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

schedule triggers can't have input parameters so we define the values we want here

runs-on: ubuntu-latest
outputs:
terraform_matrix: ${{ inputs.terraform_matrix || vars.schedule_terraform_matrix }}
provider_matrix: ${{ inputs.provider_matrix || vars.schedule_provider_matrix }}
steps:
- if: false
run: echo jobs need steps

clean-before:
secrets: inherit
uses: ./.github/workflows/cleanup-test-env.yml

mig-tests:
needs: clean-before
needs: [clean-before, versions]
if: ${{ !cancelled() }}
strategy:
max-parallel: 1
fail-fast: false
matrix:
terraform_version: ['', '1.0.8'] # '' for latest version
provider_version: ['', '1.11.1'] # '' for latest version
terraform_version: ${{ fromJSON(needs.versions.outputs.terraform_matrix) }}
provider_version: ${{ fromJSON(needs.versions.outputs.provider_matrix) }}
name: mig-tests-${{ matrix.terraform_version || 'latest' }}-${{ matrix.provider_version || 'latest' }}
secrets: inherit
uses: ./.github/workflows/migration-tests.yml
Expand All @@ -28,13 +51,13 @@ jobs:
provider_version: ${{ matrix.provider_version }}

acc-tests:
needs: mig-tests
needs: [mig-tests, versions]
if: ${{ !cancelled() }}
strategy:
max-parallel: 1
fail-fast: false
matrix:
terraform_version: ['', '1.0.8'] # '' for latest version
terraform_version: ${{ fromJSON(needs.versions.outputs.terraform_matrix) }}
name: acc-tests-${{ matrix.terraform_version || 'latest' }}
secrets: inherit
uses: ./.github/workflows/acceptance-tests.yml
Expand Down
1 change: 1 addition & 0 deletions .tool-versions
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
golang 1.21.3
terraform 1.6.6
Copy link
Member Author

@lantoli lantoli Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

opportunistic change not related to the main goal of the PR