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

Migrate ec_deployment #2

Merged

Conversation

dimuon
Copy link
Owner

@dimuon dimuon commented Sep 7, 2022

Description

ec_deployment resource migration

Limitations.
State upgrade is not currently implemented.

Few acceptance tests fail. Please see the section below.

Related Issues

Motivation and Context

How Has This Been Tested?

  • unit tests (majority of the relevant unit tests were migrated)
  • acceptance tests
  • manual testing

There are still few acceptance tests that fail:

  • there are 4 acceptance tests (their names mention version 0.4.1) that verify migration of other data sources and resources from 0.4.1 to 0.6.0. To fix them we need implement a state upgrade for ec_deployment.
  • TestAccDeploymentExtension_pluginDownload fails on 0.4.1 with the same error ("Custom plugin is not allowed at your subscription level.")
  • TestAccDeploymentExtension_bundleFile fails quite often on both 0.4.1 and the branch with the "failed to upload file" error.
  • TestAccDeployment_withExtension fails quite often with the same error as TestAccDeploymentExtension_bundleFile.

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

@dimuon dimuon force-pushed the feature/530/ec-deployment branch from dda2e9d to 4615f73 Compare September 8, 2022 13:22
@dimuon
Copy link
Owner Author

dimuon commented Sep 9, 2022

@pascal-hofmann , the schema for ec_deploment resource is more or less complete. More probably we will need to tune/fix it once CRUD operations are ready. As for now I tested it just on a small set of examples.

@pascal-hofmann
Copy link
Collaborator

@dimuon Do you need help with ec_deployment or should I start with porting one of the other resources?

@dimuon
Copy link
Owner Author

dimuon commented Sep 12, 2022

@pascal-hofmann , I think it makes sense to either start porting other resources or maybe adding acceptance tests for the migration, as recommended by Terraform - https://github.com/hashicorp/terraform-plugin-framework/blob/main/website/docs/plugin/framework/migrating/testing.mdx.

@dimuon
Copy link
Owner Author

dimuon commented Sep 13, 2022

@pascal-hofmann , I've pushed still unfinished implementation of Read for ec_deployment. I tested it on a resource that was created with the previous version. The problem is that the plan is not empty for a configuration that doesn't introduce any change (basically I've created a resource using the old provider and then executed terraform plan using the new implementation and the existing resource configuration that was untouched). I'm looking into this.

Also, it looks like changes you've suggested aren't compilable - Value property isn't available in the snippets you've commented on.

@pascal-hofmann
Copy link
Collaborator

pascal-hofmann commented Sep 13, 2022

Also, it looks like changes you've suggested aren't compilable - Value property isn't available in the snippets you've commented on.

My IDE (goland) deceived me with this. I had problems with Value.String() and assumed this is the same here, because goland found these usages. I should have tested my suggestions locally. Sorry for the confusion.

@dimuon dimuon force-pushed the feature/530/ec-deployment branch 2 times, most recently from 96d460c to b12b731 Compare September 21, 2022 08:55
Comment on lines 41 to 46
Elasticsearch []*Elasticsearch `tfsdk:"elasticsearch"`
Kibana []*Kibana `tfsdk:"kibana"`
Apm []*Apm `tfsdk:"apm"`
IntegrationsServer []*IntegrationsServer `tfsdk:"integrations_server"`
EnterpriseSearch []*EnterpriseSearch `tfsdk:"enterprise_search"`
Observability []*Observability `tfsdk:"observability"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The recommended way to deal with configuration or plan data for now is using types package types in Go structs, e.g.

type myModel struct {
  ExampleList types.List `tfsdk:"example_list"`
}

-- https://discuss.hashicorp.com/t/terraform-plugin-framework-value-conversion-error-unhandled-unknown-value/44382/3

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, it's a valid concern. It looks like there is also an option to leverage appropriate interfaces.
I'll reconsider these definitions when I complete Create/Update.
Thank you for the heads-up.

@dimuon dimuon force-pushed the feature/530/ec-deployment branch 4 times, most recently from b1f7df8 to 626143e Compare September 27, 2022 15:50
@dimuon dimuon force-pushed the feature/530/ec-deployment branch from 8167a02 to 45a2157 Compare October 20, 2022 13:25
@dimuon dimuon marked this pull request as ready for review December 2, 2022 13:41
Copy link
Collaborator

@pascal-hofmann pascal-hofmann left a comment

Choose a reason for hiding this comment

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

LGTM apart from minor things. I did not do any real world tests with the new ec_deployment resource though.

README.md Outdated Show resolved Hide resolved
ec/acc/deployment_autoscaling_test.go Outdated Show resolved Hide resolved
ec/acc/testdata/deployment_autoscaling_2.tf Outdated Show resolved Hide resolved
ec/acc/testdata/deployment_autoscaling_2.tf Outdated Show resolved Hide resolved
ec/acc/testdata/deployment_autoscaling_2.tf Outdated Show resolved Hide resolved
ec/acc/testdata/deployment_autoscaling_2.tf Outdated Show resolved Hide resolved
@dimuon dimuon changed the title WIP: Feature/530/migrate to plugin framework Feature/530/migrate to plugin framework Dec 5, 2022
@dimuon dimuon merged commit 320fb77 into feature/530/migrate-to-plugin-framework Dec 7, 2022
@dimuon dimuon changed the title Feature/530/migrate to plugin framework Migrate ec_deployment Dec 7, 2022
dimuon added a commit that referenced this pull request Feb 2, 2023
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.

5 participants