-
Notifications
You must be signed in to change notification settings - Fork 526
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
smoke: Fix smoke test Terraform issue #15901
base: main
Are you sure you want to change the base?
Conversation
description = "APM Cloud component ID" | ||
} | ||
|
||
output "fleet_component_id" { | ||
value = ec_deployment.deployment.integrations_server[0].resource_id | ||
value = var.integrations_server ? ec_deployment.deployment.integrations_server[0].resource_id : "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[For reviewers] Not sure what this should fallback to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@endorama Do you have any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the distinction between integrations_server
and apm
resource even bleeding into our code? Having one variable defined in terraform that abstracts this sound the right path forward. @ericywl you are on a good path here already, adding the fallback for apm
resource. @endorama is there any reason why apm_component_id
and fleet_component_id
would be needed? (Given that they currently point to the same resource I would not think so).
Quick recap: in cloud the old resource was apm
, and the newer one (introduced in 8.0
) is integrations_server
. Currently both can exist, for deployments created in 7.x
the resource will be apm
, for newer deployments resource will be integrations_server
(if not explicitly migrated, which we don't do here).
This pull request does not have a backport label. Could you fix it @ericywl? 🙏
|
Seems like there is still this issue (which is from before?):
|
Motivation/summary
Fix smoke test issue:
How to test these changes
Run smoke test again.
Test run: https://github.com/elastic/apm-server/actions/runs/13560141549, it seems the usual ones are passing again.