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

Always install the latest plugin versions for Terraform tests #11447

Merged
merged 1 commit into from
May 11, 2021

Conversation

hakman
Copy link
Member

@hakman hakman commented May 10, 2021

/cc @rifelpet

@k8s-ci-robot k8s-ci-robot requested a review from rifelpet May 10, 2021 14:03
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 10, 2021
@hakman
Copy link
Member Author

hakman commented May 10, 2021

/test pull-kops-verify-terraform

@rifelpet
Copy link
Member

will this mean we no longer catch situations where we use a recently-added field for a terraform resource but forget to update the minimum provider version to match?

if t.Cloud.ProviderID() == kops.CloudProviderGCE {
writeMap(requiredProvidersBody, "google", map[string]cty.Value{
"source": cty.StringVal("hashicorp/google"),
"version": cty.StringVal(">= 2.19.0"),
})
} else if t.Cloud.ProviderID() == kops.CloudProviderAWS {
writeMap(requiredProvidersBody, "aws", map[string]cty.Value{
"source": cty.StringVal("hashicorp/aws"),
"version": cty.StringVal(">= 3.34.0"),
})
if featureflag.Spotinst.Enabled() {
writeMap(requiredProvidersBody, "spotinst", map[string]cty.Value{
"source": cty.StringVal("spotinst/spotinst"),
"version": cty.StringVal(">= 1.33.0"),
})
}

@hakman
Copy link
Member Author

hakman commented May 10, 2021

Should still respect the constraints, but upgrade the plugins cached on dev machines to latest allowed.

  -upgrade=false       If installing modules (-get) or plugins, ignore
                       previously-downloaded objects and install the
                       latest version allowed within configured constraints.

@johngmyers
Copy link
Member

Should we have the tests always specify the lowest provider versions?

@johngmyers
Copy link
Member

Test twice, once with oldest and once with newest?

@hakman
Copy link
Member Author

hakman commented May 10, 2021

This is how I noticed that it doesn't work. It failed all tests for me this morning because the AWS provider was too old locally and it didn't what to do. Had to add this change to work.
Yes, I tested and respects the constraints for minimum provider version.

@hakman
Copy link
Member Author

hakman commented May 10, 2021

/retest

@rifelpet
Copy link
Member

I'm not sure I understand why it would have failed for you. If you had a version < 3.34.0 cached locally, I would expect terraform init (no -upgrade) to redownload the provider >= 3.34.0 rather than trying to use a version that doesn't match the defined requirements. If 3.34.0 isn't actually sufficient to use all of the functionality that we generate, then we should bump the minimum version.

I do like the idea of testing twice, once with the oldest and once with the newest. I'm not sure how easily we can do that though, it might require editing the required_providers block to use = instead of >=

@hakman
Copy link
Member Author

hakman commented May 10, 2021

This is how it behaves for me:

> terraform init

Initializing the backend...

Initializing provider plugins...
- Reusing previous version of hashicorp/aws from the dependency lock file

Error: Failed to query available provider packages

Could not retrieve the list of available versions for provider hashicorp/aws:
locked provider registry.terraform.io/hashicorp/aws 3.31.0 does not match
configured version constraint >= 3.34.0; must use terraform init -upgrade to
allow selection of new versions

@rifelpet
Copy link
Member

ah, that makes (a bit) more sense. does terraform init -upgrade then download 3.34.0 or the latest 3.X ?

@hakman
Copy link
Member Author

hakman commented May 10, 2021

Yup :)
If you use "= 3.30.0", it will download 3.30.0 when using -upgrade.

@rifelpet
Copy link
Member

but using >= 3.34.0 will download the latest 3.X ?

@hakman
Copy link
Member Author

hakman commented May 10, 2021

Yes, will download the latest 3.X, but it's the same as what the automated test does.

hakman@MecTroll minimal-ipv6 % terraform init -upgrade

Initializing the backend...

Initializing provider plugins...
- Finding hashicorp/aws versions matching ">= 3.32.0"...
- Installing hashicorp/aws v3.39.0...
- Installed hashicorp/aws v3.39.0 (self-signed, key ID 34365D9472D7468F)

Copy link
Member

@rifelpet rifelpet left a comment

Choose a reason for hiding this comment

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

you're right that this preserves the existing functionality of the prow job so I'm inclined to merge as-is and if we can add more testing in a separate PR with = instead of >= that would be ideal.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit fe8275e into kubernetes:master May 11, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone May 11, 2021
@hakman hakman deleted the terraform_init_upgrade branch May 11, 2021 03:19
@johngmyers johngmyers modified the milestones: v1.21, v1.22 May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants