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

feat: terraform autodiscovery #1621

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

mcwarman
Copy link
Member

@mcwarman mcwarman commented Sep 26, 2023

Fix #534

Add terraform autodiscovery support for lock files.

Test

To test this pull request, you can run the following commands:

cd pkg/plugins/autodiscovery/terraform/
go test ./...
name: Terraform Auto Discovery

autodiscovery:
  crawlers:
    terraform:
      platforms:
        - linux_amd64
        - linux_arm64
        - darwin_amd64
        - darwin_arm64

Additional Information

Tradeoff

Potential improvement

  • Support terraform/providers

@mcwarman mcwarman force-pushed the feature/terraform-auto-discovery branch from 3b0dedf to 4c601bf Compare September 26, 2023 19:50
@mcwarman mcwarman marked this pull request as ready for review September 26, 2023 19:51
@olblak
Copy link
Member

olblak commented Sep 27, 2023

While this a great PR, I have one important change and a few questions

Jsonschema

The variable AutodiscoverySpecsMapping:

AutodiscoverySpecsMapping = map[string]interface{}{

needs the mapping for Terraform similarly to the other entries, that variable is used to generate the Jsonschema spec which is ultimately used to generate the website documentation and IDE.

Default Autodiscovery

The variable DefaultCrawlerSpecs could have terraform listed there, that variable contains the default autodiscovery plugins we want to execute when we run updatecli diff --experimental. Note that every autodiscovery plugins shouldn't necessary be there as we may have collusion between plugin but I think it could be interesting in the current situation.

Platform

I did a quick test with the following manifest on https://github.com/jenkins-infra/azure

autodiscovery:
  crawlers:
    terraform:

and it complained about missing platform, I had to use

autodiscovery:
  crawlers:
    terraform:
      platforms:
        - linux_amd64
        - linux_arm64
        - darwin_amd64
        - darwin_arm64

It's been a while since I had to maintain Terraform and I don't remember the purpose of that platform setting, could it be detected? If not that's ok but then we shouldn't add the terraform plugin to the default list of plugin execute as it will always fail

@mcwarman
Copy link
Member Author

AutodiscoverySpecsMapping

I'll add that.

Default Autodiscovery

It can be added, but only if we solve Platform

Platform

When running terraform init it will detect the platform you are running on you can then add hashes for that. You can add additional platforms using terraform providers lock command.

An option is provide a sensible default maybe just linux_amd64 if nothing is specified.

@olblak
Copy link
Member

olblak commented Sep 27, 2023

Thanks for the information,

An option is provide a sensible default maybe just linux_amd64 if nothing is specified.

That's what I initially tried but it was confusing because Updatecli reported some changes such as bump version from "1.0.0" to "1.0.0" basically I had to specify the four platforms to show me the correct result.

If it's not possible to detect what platform is in used within the lock then I guess the best approach is to not run the terraform autodiscovery by default

@mcwarman
Copy link
Member Author

That's what I initially tried but it was confusing because Updatecli reported some changes such as bump version from "1.0.0" to "1.0.0" basically I had to specify the four platforms to show me the correct result.

Yeah that would be because the hashes would have been removed for the other platforms, there's probably some additional logging required for when that happens.

If it's not possible to detect what platform is in used within the lock then I guess the best approach is to not run the terraform autodiscovery by default

Detecting platforms would be expensive as I don't think there's anyway to look up hashes origin without downloading all of the artefacts and cross checking them.

Should there be a default added so the empty config works?

autodiscovery:
  crawlers:
    terraform:

@olblak
Copy link
Member

olblak commented Sep 27, 2023

I think I would do something like

if len(platforms) == 0 { 
  logrus.Warningf("No platforms specified, we fallback to linux_amd64, linux_arm64, darwin_amd64, darwin_arm64")
  set default value
}

@mcwarman
Copy link
Member Author

mcwarman commented Sep 27, 2023

Also added log message:

⚠ - "hashicorp/random" already set to "3.5.1", hashes for the provider have been updated in file ".terraform.lock.hcl"

@olblak
Copy link
Member

olblak commented Sep 27, 2023

Thanks for the quick feedback, I just did some testing and it works great

@olblak
Copy link
Member

olblak commented Sep 28, 2023

Thanks @mcwarman for the PR

@olblak olblak merged commit 8f0c24b into updatecli:main Sep 28, 2023
6 checks passed
@olblak olblak added the enhancement New feature or request label Sep 28, 2023
@olblak olblak added this to the 0.62.0 milestone Sep 28, 2023
@olblak
Copy link
Member

olblak commented Sep 28, 2023

@mcwarman I almost forgot but that would be great to have some documentation on https://github.com/updatecli/website/tree/master/content/en/docs/plugins/autodiscovery

@mcwarman mcwarman deleted the feature/terraform-auto-discovery branch September 28, 2023 08:06
@mcwarman
Copy link
Member Author

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

Successfully merging this pull request may close these issues.

Feature Request: terraform support
2 participants