Remove validators.Known() on endpoint.

GeorgeGkinis commented 1 month ago

This removes validators.Known() from the ec.endpoint parameter. This will allow setting ec.endpoint as a variable and prevents an error "Value must be known" during terraform plan

Description

We want to dynamically change the endpoint URL using variables during CI/CD deployments. It is currently not possible because during terraform plan ec.endpoint is not filled with the given variable:

provider "ec" {
  endpoint = var.ECE_CONSOLE_URL
  apikey = var.ECE_CONSOLE_API_KEY
}

local.auto.tfvars:

ECE_CONSOLE_URL = "https://console.ece.mydomain.com"
ECE_CONSOLE_API_KEY = "myapikey"

The endpoint field is also declared as optional in the code and according to documentation has a default value of "https://api.elastic-cloud.com"

Motivation and Context

How Has This Been Tested?

Types of Changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Refactoring (improves code quality but has no user-facing effect)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Documentation

Readiness Checklist

[ ] My code follows the code style of this project
[ ] My change requires a change to the documentation
[ ] I have updated the documentation accordingly
[ ] I have added tests to cover my changes
[ ] All new and existing tests passed

cla-checker-service[bot] commented 1 month ago

💚 CLA has been signed

GeorgeGkinis commented 1 month ago

@tobio @gigerdo Could you please take a look at this? I am curious why validators.Known() was needed for ec.endpoint and as such if this is a breaking change. I suppose not, because everyone had to explicitly set ec.endpoint up until now.

Maybe this should be fixed in validators.Known()? If there is a default value the Known validator should always return no problem?

Thanks!

gigerdo commented 1 month ago

Yes, I think you are right. It should be fine to remove the validation, so it works to use variables in the config.

cc @dimuon

GeorgeGkinis commented 1 month ago

@gigerdo I believe the regression test error is unrelated to my change?

dimuon commented 1 month ago

@GeorgeGkinis , looks like it caused by https://github.com/hashicorp/terraform-provider-aws/issues/39676.

GeorgeGkinis commented 1 month ago

@dimuon Should I make another pull request to implement the workaround in providers.tf or should we wait until it is resolved by Hashicorp?

dimuon commented 1 month ago

@GeorgeGkinis let's proceed with the workaround. Thanks for the contribution!

GeorgeGkinis commented 1 month ago

@dimuon Check https://github.com/elastic/terraform-provider-ec/pull/867

GeorgeGkinis commented 1 month ago

@dimuon Maybe try running these test once again? I rebased this branch on top of master with https://github.com/elastic/terraform-provider-ec/pull/867

GeorgeGkinis commented 1 month ago

@dimuon Njoy the weekend!

dimuon commented 1 month ago

Thank you @GeorgeGkinis, wishing you a great weekend too!

GeorgeGkinis commented 3 weeks ago

@dimuon Is there a known release date with this fix? We cannot deploy from our CI/CD until this is released, but we can wait a week or 2.

dimuon commented 3 weeks ago

@GeorgeGkinis , there is no specific date for the new release but I suppose we can release the fix soon.

elastic / terraform-provider-ec