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

Introduce integrations_server resource #425

Merged
merged 29 commits into from
Feb 24, 2022

Conversation

mieciu
Copy link
Contributor

@mieciu mieciu commented Jan 19, 2022

This pull request adds a new deployment resource support - integrations_server introduced in Elastic stack version 8.0.0.

Description

Users can use the new integrations_server resource in their *.tf files just like any other stateless resources (e.g.kibana, enterprise_search).

Example:

resource "ec_deployment" "example" {
  name                   = "my-deployment"
  region                 = "us-east-1"
  version                = "8.0.0"
  deployment_template_id = "aws-io-optimized-v2"

  elasticsearch {}

  kibana {}

  integrations_server {
    topology {
      zone_count = 2
    }
  }
}

How Has This Been Tested?

Tested manually, unit/acceptance tests have been added as well.

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

@mieciu mieciu force-pushed the integrations-server-support branch from e7e731d to 51ed9b9 Compare January 20, 2022 13:31
@mieciu mieciu marked this pull request as ready for review January 31, 2022 13:37
@mieciu mieciu requested a review from a team as a code owner January 31, 2022 13:37
@mieciu mieciu force-pushed the integrations-server-support branch from 3e9dada to 4f8ad72 Compare January 31, 2022 16:07
@andrew-moldovan
Copy link
Contributor

@mieciu just checking in on the status of this

marclop and others added 4 commits February 8, 2022 14:30
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@mieciu mieciu requested a review from a team as a code owner February 14, 2022 02:30
@marclop
Copy link
Contributor

marclop commented Feb 14, 2022

Before we merge this in, we should release a new version of the cloud-sdk-go (1.8.1) and update the go.mod dependency to it. Also, we should write an acceptance test that validates that a deployment with an integrations_server can be created and updated.

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Nothing major, I've pushed a couple of fixes related to the failing tests. Before we can merge this, we should also update the docs/ and add the following:

  • Example of integrations server in ec_deployment, when to use it and how it differs to the apm resource.
  • Update the ec_deployment resource adding the new attributes and fields.
  • Update the ec_deployment data source adding the new attributes.

Last, we should add an acceptance test case which creates and updates the new integrations_server, for example:

  • Create a 1g integrations_server deployment.
  • Update to 2g.

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

It seems that you removed all documentation related to the apm resource, I would prefer if we left most of the apm references as is and made clear that from 8.0.0 onwards users should use the integrations_server instead of apm.

Otherwise, this change will lead to integrations_server being used for all versions, which isn't supported and isn't the intended use case.

Could you also write new acceptance tests to cover the integrations_server resource? We're missing those and would be great to have. I believe we should create two acceptance test cases:

  1. Create a deployment with an integrations_server from scratch (using latest).
    1.1. Update some settings
  2. Create an APM Deployment with an apm resource (using 7.17.*)
    2.1. Update the deployment to 8.0.0.
    2.2. Once updated, use the integrations_server resource instead.

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

We should ideally also add some unit tests for all the flattening and expanding of the integrations server like we do for all the other resources

docs/data-sources/ec_deployments.md Outdated Show resolved Hide resolved
docs/resources/ec_deployment.md Outdated Show resolved Hide resolved
ec/ecdatasource/deploymentsdatasource/datasource_test.go Outdated Show resolved Hide resolved
ec/ecdatasource/deploymentsdatasource/datasource_test.go Outdated Show resolved Hide resolved
ec/ecdatasource/deploymentsdatasource/datasource_test.go Outdated Show resolved Hide resolved
@mieciu mieciu requested review from marclop and removed request for a team February 22, 2022 15:01
@mieciu
Copy link
Contributor Author

mieciu commented Feb 22, 2022

@marclop I believe all your concerns have been addressed, please give this another look 🙇

@mieciu mieciu dismissed marclop’s stale review February 22, 2022 15:31

all your concerns have been addressed

@mieciu
Copy link
Contributor Author

mieciu commented Feb 22, 2022

16:40:20  === CONT  TestAccDeployment_snapshot_restore
16:40:20      deployment_snapshot_test.go:39: Step 2/3 error: Error running apply: exit status 1

looks completely unrelated to my changes, perhaps that's a flakiness?

@marclop
Copy link
Contributor

marclop commented Feb 23, 2022

That seems to be the case, I've opened #444 which we should merge and investigate why the test fails separately.

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for all the changes and the added tests.

@simitt
Copy link

simitt commented Feb 23, 2022

Thanks @mieciu for making the terraform provider work with the new integrations_server!

@marclop
Copy link
Contributor

marclop commented Feb 24, 2022

I forgot to mention that we should add a changelog entry in https://github.com/elastic/terraform-provider-ec/tree/master/.changelog.

@mieciu mieciu merged commit 20a7b11 into elastic:master Feb 24, 2022
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.

None yet

4 participants