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

If secret has been deleted from vault, drop it from the state. #56

Closed
wants to merge 2 commits into from

Conversation

tomwilkie
Copy link

@tomwilkie tomwilkie commented Jan 11, 2018

I cribbed this from https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_ami.go#L151, might not be the right way of signalling the resource has been deleted.

Fixes #55

@apparentlymart
Copy link
Contributor

Hi @tomwilkie! Thanks for working on this.

This is indeed the current way to signal to Terraform that the object referenced by a resource has been deleted. Along with this, we usually like to have a debug log explaining what's happened here, in case this happens erroneously in a case where returning an error would be more appropriate and we consult the logs to figure out why. (aws_ami is, unfortunately, not a good example here. 😖)

    log.Printf("vault_generic_secret at %s has been deleted, so removing it from state", path)

@tomwilkie
Copy link
Author

Thanks for the review @apparentlymart - I've add the log line.

@adamdecaf
Copy link

adamdecaf commented Jan 29, 2018

LGTM. We ran into this and I had the same patch. Could we get this deployed?

A quick glance at the issues almost looks like there are many reports of this: #1, #50, #55, #58

@paddycarver
Copy link
Contributor

I'm happy to merge this, but ideally I'd like to see an acceptance test that would catch this behaviour. I'll put a reminder to follow up on this again at the end of the week, and I'll write the test myself and find a reviewer if nobody has had the time to get to it by then.

@paddycarver paddycarver self-assigned this Feb 8, 2018
@paddycarver paddycarver self-requested a review February 8, 2018 23:59
@paultyng
Copy link
Contributor

@tomwilkie sorry I missed this when I just wrote up #83, but it should be covered now with the fix and an acceptance test.

@paultyng paultyng closed this Mar 12, 2018
@paddycarver
Copy link
Contributor

Hey @tomwilkie, just wanted to echo @paultyng here: thanks so much for contributing to Terraform, and my apologies for not picking this one up in a timelier manner. And then it slipped my mind this morning while reviewing #83. Really do appreciate your time and effort here, and my sincerest apologies for the oversight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants