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

set logpush job id to "" when not found #798

Merged
merged 2 commits into from
Sep 22, 2020

Conversation

trjstewart
Copy link
Contributor

fixes #797

There are currently no tests for the logpush_job resource that I could find so I haven't added or amended any. I assume that is because it requires resources external to Cloudflare?

Either way I have build the provider with this change and tested it ourselves and it's working as intended.

@jacobbednarz
Copy link
Member

Thanks for putting in the time to fix this one 🍰 As the scenario we're addressing here is when the logpush job isn't in the remote but still in the state, we should include a fix for #795 where attempting to delete the resource fails due to it not being present in the remote. This should then cover off all the cases where the remote is out of sync with the local state.

@trjstewart
Copy link
Contributor Author

Good catch @jacobbednarz, I must be blind because I completely missed that issue. For now this small patch solved our immediate issue and we're just running a custom build of this branch internally for the time being. I won't likely have the time in the next few days to address anything additional for #795 so anyone else is more than welcome to.

Should someone delete a resource outside of Terraform, it won't be found
when the remote delete is called. Instead of throwing an error stating
we couldn't find it, just remove it from the state.

Fixes cloudflare#795
return fmt.Errorf("error deleting logpush job: %+v", job.ID)
}

return resourceCloudflareLogpushJobRead(d, meta)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to call Read as there isn't anything to "sync" after we d.SetId("").

@jacobbednarz jacobbednarz merged commit a915424 into cloudflare:master Sep 22, 2020
boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 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
2 participants