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

Remove the Terraform CLI from the provider package and deprecate the relevant provider command-line flags #434

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Dec 13, 2023

Description of your changes

With #424, we have switched to the Terraform plugin SDK-based reconciliation for all the available managed resources in this provider. This PR removes the Terraform CLI and the Terraform Google provider binaries from the built provider package. We still need to generate the config/schema.json for diff detection purposes and for the code generation pipeline, and we are planning to address the issue of generating the schema.json without the TF CLI in a follow-up PR.

We are also deprecating the following process-forking related command-line arguments:

  • terraform-version
  • terraform-provider-version
  • terraform-native-provider-path
  • terraform-provider-source
  • provider-ttl

What the user will see in the provider logs if she uses any of these flags is something like the following:

warning: Command-line flag "terraform-version" is deprecated and no longer used. It will be removed in a future release. Please remove it from all of your configurations (ControllerConfigs, etc.).
warning: Command-line flag "terraform-provider-source" is deprecated and no longer used. It will be removed in a future release. Please remove it from all of your configurations (ControllerConfigs, etc.).
warning: Command-line flag "terraform-provider-version" is deprecated and no longer used. It will be removed in a future release. Please remove it from all of your configurations (ControllerConfigs, etc.).
warning: Command-line flag "provider-ttl" is deprecated and no longer used. It will be removed in a future release. Please remove it from all of your configurations (ControllerConfigs, etc.).

These logs will be available even if the debug logging is not enabled for the provider.

This PR also bumps the --max-reconcile-rate command-line option's default to 100 to leverage the SDK-based reconciliation runtime's performance improvements.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Via uptest runs and manually with index.docker.io/ulucinar/provider-gcp-cloudplatform:v0.40.0-208a3a3a33d002c712baed74f23805b25d64805c.

deprecate the relevant provider command-line flags.

- Increase the default value `--max-reconcile-rate` from 10 to 100
- Deprecate the following provider command-line flags:
  - terraform-version
  - terraform-provider-version
  - terraform-native-provider-path
  - terraform-provider-source
  - provider-ttl

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/cloudplatform/serviceaccountkey.yaml"

@sergenyalcin
Copy link
Collaborator

/test-examples="examples/storage/bucketacl.yaml"

@sergenyalcin
Copy link
Collaborator

/test-examples="examples/sql/instance.yaml"

@sergenyalcin
Copy link
Collaborator

/test-examples="examples/cloudplatform/serviceaccount.yaml"

@ulucinar
Copy link
Collaborator Author

I've also verified the UserAgent HTTP header we are passing to the SDK requests. It's currently something like Terraform/ (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/dev upbound-provider-gcp/Terraform/ (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/dev upbound-provider-gcp/v0.40.0-208a3a3a33d002c712baed74f23805b25d64805c when the provider is run locally.

Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar LGTM! I left a few comments to document some points.

export TERRAFORM_PROVIDER_VERSION := 4.77.0
export TERRAFORM_PROVIDER_DOWNLOAD_NAME := terraform-provider-google
export TERRAFORM_PROVIDER_DOWNLOAD_URL_PREFIX := https://releases.hashicorp.com/terraform-provider-google/$(TERRAFORM_PROVIDER_VERSION)
export TERRAFORM_PROVIDER_SOURCE := hashicorp/google
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @sergenyalcin,
We now use TERRAFORM_PROVIDER_SOURCE and the like while extracting the Terraform provider metadata and for generating the schema.json. However, it's no longer used at runtime or for downloading the Terraform provider image while building the provider package.

return func(ctx context.Context, client client.Client, mg resource.Managed) (terraform.Setup, error) {
ps := terraform.Setup{
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we leave the TF CLI base reconciliations for every upjet-based provider, do we consider deprecating and removing these setup fields: Version, Requirement, Scheduler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we may keep them for the other community upjet-based providers. Maintainers of those providers may prefer to integrate via the CLI because that integration happens at a higher layer.

@ulucinar ulucinar merged commit 643628c into crossplane-contrib:main Dec 13, 2023
13 checks passed
@ulucinar ulucinar deleted the remove-tf-cli branch December 13, 2023 14:54
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