Feature/530/migrate to plugin framework #567

dimuon · 2022-12-07T09:52:36Z

Description

Migration to TF Plugin Framework.
New schema for ec_deployment.

Limitations

State upgrade is not currently implemented for ec_deployment. The recommended way to port existing resources to the new provider version is state import.
The migration is based on the provider version 0.4.1. We plan to bring changes from 0.5.0 and master branch that were merged after the split (e.g. [feature] support template hardware migration API #487) in future releases.

Review notes
The change is quite big. The alternative way to review the change is reviewing PRs in the migration repo. Usually, it was a PR per resource/datasource, e.g. this PR migrated ec_deployment. Also, mind the fact that there are a number of JSON files with resources for acceptance and unit tests that contributed to the size of the PR.

Some design decisions were motivated by this TF issue that prevents from, e.g., using Computed + Optional nested attributes with underlying Computed attributes (e.g. deploymentresource.elasticsearch.topology.autoscaling is defined as Required while ideally it should be Optional + Computed).

Also, the current TF core logic makes it nearly impossible to use sets with computed + optional underlying attributes that means, in particular, that Elasticsearch topology (where each tier has computed and optional attributes) cannot be represented using sets (tfsdk.SetNestedAttributes). More details:

Related Issues

#506
#530

Motivation and Context

The change should resolve a number of Elasticsearch topology related issues in ec_deployment, e.g.
#336, #445, #466, #467.

The Plugin Framework represents TF vision of the future of plugin development.
So the migration was a question of time that got a priority with the idea to resolve a number of mentioned topology related issues.

How Has This Been Tested?

unit tests
acceptance tests
manual testing

There are 4 new acceptance tests that are disabled now. Prior to enabling them we need implement state upgrade for ec_deployment.

Types of Changes

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

Introduce mux and port the provider to v6.

Migrate data sources to terraform-provider-framework

…m-provider-framework

Migrate resource ec_deployment_traffic_filter_association to terraform-provider-framework

…ork-0.12 Update to terraform-plugin-framework 0.12 and fix import order

…ramework The behavior of the default_value plan_modifier changes, so it also applies default values when the value has been explicitly specified before. Unit test timeout increased. Switch to v6 provider in tests.

Migrate resource ec_deployment_traffic_filter to terraform-provider-framework

* Use ListNestedAttributes for nested schemas in datasource ec_stack * Use ListNestedAttributes for nested schemas in datasource ec_deployment * Use blocks and ListNestedAttributes for nested schemas in datasource ec_deployments

…ugin-framework (#10) Co-authored-by: Pascal Hofmann <[email protected]>

…ation tests

Update dependencies and remove external provider workaround from migration tests

#12) * Migrate resource ec_deployment_extension to terraform-plugin-framework * Remove useless use of ec.Bool() and ec.String() Co-authored-by: Pascal Hofmann <[email protected]>

cla-checker-service · 2022-12-07T09:52:42Z

💚 CLA has been signed

tobio · 2022-12-07T11:01:05Z

The recommended way to port existing resources to the new provider version is state import.

It's not possible to import the elastic password, this feels like a pretty big foot gun that we're handing users.

tobio · 2022-12-07T11:03:20Z

To be honest, this is impossible to actually review. Can we instead break this up so we're only migrating a single resource/data source per PR?

dimuon · 2022-12-07T11:12:55Z

To be honest, this is impossible to actually review. Can we instead break this up so we're only migrating a single resource/data source per PR?

Yes, it's quite difficult to review this. We did some reviews with @pascal-hofmann when we were working on the feature branch and basically we followed the way that you proposed. But, even in that case, it's not so easy - ec_deployment code base is quite big by its own, e.g. this was the PR for ec_deployment only. However, there are also a number of json files for acceptance and unit tests.

dimuon · 2022-12-07T11:16:45Z

To be honest, this is impossible to actually review. Can we instead break this up so we're only migrating a single resource/data source per PR?

Also, it means additional code changes to keep the provider working. As I said, we did this during the work on the feature branch.

As an alternative, we can review the PRs on the migration repo - usually, it were a PR per resource/datasource.

tobio

Tiny way through this change.

README.md

ec/provider.go

ec/internal/util/parsers.go

ec/internal/util/helpers.go

ec/internal/planmodifier/default_value.go

ec/internal/converters/extract_endpoint.go

ec/internal/converters/convert_tags.go

…te-to-plugin-framework

pascal-hofmann · 2023-02-10T08:29:47Z

As far as I know the CLA has been signed. I guess my comment will re-trigger the CLA check?

tobio · 2023-02-15T00:47:32Z

@pascal-hofmann I can see you've signed the CLA, however it looks like these commits are using a different email address ([email protected]), not tied to your GH account.

2194ad3
8a79970
c197f28
b00e28a

@dimuon I guess you could reset the author on the above commits to use [email protected] in order to get past the check. FWIW manually triggering the CLA checker on this PR timed out, maybe there's a longer timeout on the automatic status check or we just force merge it 🤷.

pascal-hofmann · 2023-02-15T07:58:26Z

I've added the address to my github account. I don't think rewriting the commits is the right thing to do.

(There is a company CLA signed with [email protected])

dimuon · 2023-02-15T08:12:11Z

I've just rerun the checker. It looks good know 👍

dimuon · 2023-02-15T08:21:00Z

I still would like to update the TF Framework to the latest version (v.1.1.1) before the merge. I've started working on this but it'll take some time - there are a number of breaking changes between 0.14.0 and 1.1.1.

tobio · 2023-02-15T08:30:43Z

https://www.youtube.com/watch?v=fR4HjTH_fTM

…te-to-plugin-framework

dimuon · 2023-02-22T16:34:51Z

@tobio , I've updated the framework to v1.1.1. Would you mind to have a look at the commit?

tobio

LGTM. Some minor nits. Some cases where we should be following 'accept interfaces, return structs'

ec/ecdatasource/deploymentdatasource/flatteners_elasticsearch.go

ec/ecdatasource/deploymentsdatasource/schema.go

ec/ecresource/deploymentresource/elasticsearch/v2/node_roles_plan_modifier.go

ec/ecresource/deploymentresource/elasticsearch/v2/node_types_plan_modifier.go

ec/ecresource/deploymentresource/observability/v1/schema.go

ec/internal/planmodifiers/bool_default_value.go

ec/internal/planmodifiers/string_default_value.go

ec/internal/validators/knownvalidator.go

ec/internal/validators/urlvalidator.go

dimuon · 2023-02-24T11:15:53Z

I’ve updated the TF framework version till the latest one (v1.1.1) and came across a defect in the Framework that prevents us from releasing based on v1.1.1 (see our acceptance test failures - all errors with elasticsearch.trust_account's unexpected new value are caused by the defect). The defect is already fixed in the Framework main branch and should be in v1.2.0.

I plan to revert the latest commits that updates the Framework version and release the provider based on Framework v0.14.0 that was used during the development and was tested the most.

We will update the TF Framework till v1.2.0 in the subsequent releases.

This reverts commits * 06bf06e * 432ce9b We have to wait for a fix for hashicorp/terraform-plugin-framework#644. It should be in Framework v1.2.0.

joelsdc · 2023-03-01T23:28:25Z

This merge breaks datasources. Please undo or fix ASAP.

dimuon · 2023-03-02T08:52:49Z

This merge breaks datasources. Please undo or fix ASAP.

@joelsdc , can you please provide more details? What is the error message? Also please share your configuration.

dimuon · 2023-03-02T08:55:16Z

@joelsdc , can you please also share the version of the Terraform CLI.

joelsdc · 2023-03-02T19:34:28Z

Hi @dimuon, I've opened issue #595

I'm using:

Terraform v1.3.9
on darwin_arm64

dimuon and others added 18 commits September 6, 2022 14:33

Introduce mux and port the provider to v6.

0627d93

Update required version of CLI in examples

2213f86

Increase TF verson in github workflow

cf373db

Merge pull request #1 from dimuon/feature/530/ec-deployment

043bbe7

Introduce mux and port the provider to v6.

Migrate data sources to terraform-provider-framework

2194ad3

Merge pull request #3 from dimuon/feature/530/port-datasources

1c61d77

Migrate data sources to terraform-provider-framework

Migrate resource ec_deployment_traffic_filter_association to terrafor…

a5de6e9

…m-provider-framework

Merge pull request #5 from dimuon/feature/530/ec-trafficfilterassoc-only

b7c8e23

Migrate resource ec_deployment_traffic_filter_association to terraform-provider-framework

Update to terraform-plugin-framework 0.12 and fix import order

8a79970

Merge pull request #6 from dimuon/feature/530/terraform-plugin-framew…

6136a1c

…ork-0.12 Update to terraform-plugin-framework 0.12 and fix import order

Merge pull request #8 from dimuon/feature/530/trafficfilterresource

c4aebde

Migrate resource ec_deployment_traffic_filter to terraform-provider-framework

Migrate resource ec_deployment_elasticsearch_keystore to terraform-pl…

993d47d

…ugin-framework (#10) Co-authored-by: Pascal Hofmann <[email protected]>

Update dependencies and remove external provider workaround from migr…

b00e28a

…ation tests

Merge pull request #11 from dimuon/update-dependencies

07aecc1

Update dependencies and remove external provider workaround from migration tests

Migrate resource ec_deployment_extension to terraform-plugin-framework (

f310127

#12) * Migrate resource ec_deployment_extension to terraform-plugin-framework * Remove useless use of ec.Bool() and ec.String() Co-authored-by: Pascal Hofmann <[email protected]>

Feature/530/migrate to plugin framework (#2)

320fb77

dimuon requested review from alaudazzi, nrichers and a team as code owners December 7, 2022 09:52

skip acceptance tests for checking upgrade from 0.4.1

40f4e5d

dimuon marked this pull request as draft December 7, 2022 11:44

tobio reviewed Dec 7, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/530/migra…

9d7c37b

…te-to-plugin-framework

tobio approved these changes Feb 2, 2023

View reviewed changes

dimuon added 2 commits February 6, 2023 12:05

fix regex expressions in acc tests for pre node_roles

1bfd39e

explicit separation between plan and state in read

b420c80

dimuon added 2 commits February 22, 2023 17:12

Update TF Framework to 1.1.1

06bf06e

Merge remote-tracking branch 'upstream/master' into feature/530/migra…

172c654

…te-to-plugin-framework

tobio reviewed Feb 22, 2023

View reviewed changes

Address PR review comments

432ce9b

dimuon added 2 commits February 24, 2023 14:16

Revert "Update TF Framework to 1.1.1" and "Address PR review comments"

9b6cc51

This reverts commits * 06bf06e * 432ce9b We have to wait for a fix for hashicorp/terraform-plugin-framework#644. It should be in Framework v1.2.0.

Update README and CHANGELOG

35c633c

dimuon merged commit 269648d into elastic:master Feb 27, 2023

pascal-hofmann mentioned this pull request Mar 22, 2023

Consider updating dependencies to Plugin Framework v1.1.1 #607

Closed

tobio deleted the feature/530/migrate-to-plugin-framework branch May 30, 2023 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/530/migrate to plugin framework #567

Feature/530/migrate to plugin framework #567

dimuon commented Dec 7, 2022 •

edited

Loading

cla-checker-service bot commented Dec 7, 2022 •

edited

Loading

tobio commented Dec 7, 2022

tobio commented Dec 7, 2022

dimuon commented Dec 7, 2022

dimuon commented Dec 7, 2022

tobio left a comment

pascal-hofmann commented Feb 10, 2023

tobio commented Feb 15, 2023

pascal-hofmann commented Feb 15, 2023 •

edited

Loading

dimuon commented Feb 15, 2023

dimuon commented Feb 15, 2023

tobio commented Feb 15, 2023

dimuon commented Feb 22, 2023

tobio left a comment

dimuon commented Feb 24, 2023

joelsdc commented Mar 1, 2023

dimuon commented Mar 2, 2023

dimuon commented Mar 2, 2023

joelsdc commented Mar 2, 2023

Feature/530/migrate to plugin framework #567

Feature/530/migrate to plugin framework #567

Conversation

dimuon commented Dec 7, 2022 • edited Loading

Description

Related Issues

Motivation and Context

How Has This Been Tested?

Types of Changes

Readiness Checklist

cla-checker-service bot commented Dec 7, 2022 • edited Loading

tobio commented Dec 7, 2022

tobio commented Dec 7, 2022

dimuon commented Dec 7, 2022

dimuon commented Dec 7, 2022

tobio left a comment

Choose a reason for hiding this comment

pascal-hofmann commented Feb 10, 2023

tobio commented Feb 15, 2023

pascal-hofmann commented Feb 15, 2023 • edited Loading

dimuon commented Feb 15, 2023

dimuon commented Feb 15, 2023

tobio commented Feb 15, 2023

dimuon commented Feb 22, 2023

tobio left a comment

Choose a reason for hiding this comment

dimuon commented Feb 24, 2023

joelsdc commented Mar 1, 2023

dimuon commented Mar 2, 2023

dimuon commented Mar 2, 2023

joelsdc commented Mar 2, 2023

dimuon commented Dec 7, 2022 •

edited

Loading

cla-checker-service bot commented Dec 7, 2022 •

edited

Loading

pascal-hofmann commented Feb 15, 2023 •

edited

Loading