Add drift handling

[samuel-br]

Issue & Steps to Reproduce / Feature Request

fixes #225

Solution

Add check if deleted in read function to determine if drift happened , and invoke destroy that remove the resource from the state, instead of throwing error

[yaronya]

Nice work @samuel-br
Left some comments/questions

Please also see my comments about the missing test case. It looks like you're missing this tests in all relevant resources

Raz has some additional comments

[razbensimon]

#219 (comment)
according to that comment, I am wondering why we don't have any call to d.IsNewResource ?
general behavior IIUC that drift flow - is when d.IsNewResource is true, we should fail/error when exists.
if d.IsNewResource is false AND we got some relevant Http Code (eg. NotFound) we should understand it as drift and call SetId("").

I'm not 100% sure about what I said, and just raise a flag for you, because that's what I understood from the TF Read examples over the web (eg AWS).
also I saw that maybe it can help.

@samuel-br just validate that please, and QA your changes by running the provider after drift changes (in env0).
don't hesitate to ask for help if needed.

[samuel-br]

I think you right but as you said i want to validate it, the explenation i think, its because the d.isnewresource is true when we plan a resource its pushing it to TF state, but its not yet created on the "real world" and then every read will get exeption, but what i dont understand is how we dont use this for now and there is still no error when apply a new resource.

[razbensimon]

@samuel-br I am not sure i understood your comment.
also please write a propper description of that PR.

@yaronya @eranelbaz please revisit my comments here.
I am not sure about it...

[razbensimon]

if !d.IsNewResource() && !found {
		log.Printf("[WARN] Project (%s) not found, removing from state", d.Id())
		d.SetId("")
		return nil
	}

Looks better to me. I took it from S3 bucket provider as example / reference.
Please do the same for others if can.

I am still not sure about this one.
IIUC we should still throw on d.IsNewResource()==true && someErrorOrBadCodeExists.
I don't see any error returned on that case. @yaronya maybe you can revisit my thoughts here?

[eranelbaz]

i don't think we need to throw something, but we do need to return nil here

[yaronya]

Yes, @eranelbaz is correct - we don't need to throw anything. This is what we wanted to avoid in first place.
We should be removing the resource from state.

@samuel-br haven't we previously discussed it and said that we don't really need to check whether IsNewResource because TF doesn't read the resource before create (only before update/delete) so this would always return false?

[razbensimon]

@yaronya i think read is invoked on create too..

[eranelbaz]

read invokes on terraform refresh IIRC

[eranelbaz]

@razbensimon did you mean to fix the if or for the log?

[razbensimon]

the if is good now, just suggested adding logs to the user as I saw in the S3 bucket example.
for your decision @samuel-br

[samuel-br]

"drift detected: TerraForm will remove {this resource} from state" is that good wording?

[razbensimon]

"[WARN] Drift Detected: Terraform will remove {this resource} from state"

[samuel-br]

c2ec119

[razbensimon]

don't we need to use it on the logic somewhere?
just wondering

[yaronya]

The PR handled also environment resource at start but since it isn't so trivial to re-create an environment on some drift and there's a deeper product decision to make before implementing it, we've decided to ditch it for now and create a separate issue for environment and project resources.
This looks like a leftover tho.

@samuel-br did you create that seperate issue btw?

[eranelbaz]

@yaronya #225?

[yaronya]

I'm not so sure. I expect to see an issue that includes environment and project resources in its description, but couldn't find it.

If I get that correctly:

• Handle drift - general #205 talks about our drift issues in general
• Handle drift - specific resources #225 tries to tackle specific resources, such as template for e.g
• We're missing another issue that tackles environment and project

[samuel-br]

nope, Ill do it later today, but I want to summary this before on channel, cause for now there is three or for types of resources in this contexts of problem

[eranelbaz]

@samuel-br maybe just change their title?
Handle drift - general
Handle drift - specific resources
etc

and link them

[razbensimon]

i am confused with all of those issues.
I just asked to make the PR with a good description (copy-paste the issue if needed to the PR).

anyways, if that line of code is related to this PR keep it.
if not - remove it

@samuel-br
cc @eranelbaz

[yaronya]

I've changed the title for #250 already FYI.
And yes, prefer not having multiple issues with the same name.

[yaronya]

@samuel-br can you please share on the main issue (i.e #205) what's needed to be done basically and why it differs from the other issues (parsing the error codes from http client layer and etc.)

[samuel-br]

@yaronya @eranelbaz I changed the issues names (@yaronya thanks on #250 ) and add #205 (comment) on the main issue, please take a look on that comment to ensure its clear enough