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

Fix a panic that could occur if no ECS Cluster was found for a given … #3829

Merged
merged 1 commit into from
Nov 10, 2015

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Nov 9, 2015

…cluster name

Fixes #3826 by looping the results and only setting if we find a match for that name

@radeksimko
Copy link
Member

This reminds me I should probably do the same thing for ECS service https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_ecs_service.go#L159

Is this now a preferred approach to deal with manual interventions? I'm just thinking it may cause strange situations when user chooses wrong set of credentials (different AWS account) or just wrong region and then wipes out the resource from state by just running terraform plan/refresh.

If we accept such edge cases, then this LGTM.

@catsby
Copy link
Contributor Author

catsby commented Nov 10, 2015

Is this now a preferred approach to deal with manual interventions?

It seems heavy handed, doesn't it. We have this established behavior of "it's not found, mark it removed" in some other places, ideally so that they are then recreated, but it does seem... wrong here. At least that's how I felt. I'd like @phinze 's opinion here as well.

@catsby
Copy link
Contributor Author

catsby commented Nov 10, 2015

Hey @radeksimko – I chatted with Paul a bit and we agree that the behavior here is correct. Regarding loss of state, those should be intact in the terraform.tfstate.backup file.

The main reasonsing here is that loss of resources like this, outside of the edge case of different credentials or changing a region, will prevent a user from moving forward with a plan. They'll be blocked until they remove the resource from the config or the statefile, or both.

Thanks for bringing this up 😄

Will you be doing this kind of update for ECS service or should I pick that up?

@phinze
Copy link
Contributor

phinze commented Nov 10, 2015

LGTM!

catsby added a commit that referenced this pull request Nov 10, 2015
provider/aws: Fix issue that could occur if no ECS Cluster was found for a give name
@catsby catsby merged commit 9e93f65 into master Nov 10, 2015
@catsby catsby deleted the b-aws-ecs-cluster-read branch November 10, 2015 22:43
@radeksimko
Copy link
Member

Will you be doing this kind of update for ECS service or should I pick that up?

I can do the update, but not until #3828 gets merged, to avoid potential conflicts. 😉

@ghost
Copy link

ghost commented Apr 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in aws_ecs_cluster read with missing ECS cluster
3 participants