search

phillbaker / terraform-provider-mailgunv3

mailgun provider for terraform based on v3 of the api
MIT License
10 stars 3 forks source link

Improvements #7

Closed invidian closed 5 years ago

invidian commented 5 years ago

I didn't have time to test it properly yet (that's why WIP), but still would like to share this work, so maybe someone else can test it as well and start using it.

Closes #2

phillbaker commented 5 years ago

Thanks, I won't be able to look at this immediately, but this looks like a great update.

On Wed, Apr 3, 2019 at 5:05 AM Mateusz Gozdek notifications@github.com wrote:

I didn't have time to test it properly yet (that's why WIP), but still would like to share this work, so maybe someone else can test it as well and start using it.

Closes #2 https://github.com/phillbaker/terraform-provider-mailgunv3/issues/2

You can view, comment on, or merge this pull request online at:

https://github.com/phillbaker/terraform-provider-mailgunv3/pull/7 Commit Summary

  • Remove commented code and format all files with gofmt
  • Update dependencies
  • Add ability to set API base URL

File Changes

Patch Links:

- https://github.com/phillbaker/terraform-provider-mailgunv3/pull/7.patch

https://github.com/phillbaker/terraform-provider-mailgunv3/pull/7.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/phillbaker/terraform-provider-mailgunv3/pull/7, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFxKWcdYvbaHHtQbsRfbgMjzuhZOWlDks5vc8Y3gaJpZM4cZH5H .

invidian commented 5 years ago

So, I've made some tests and found few bugs, which are fixed now.

I'm not sure yet about parts with context, I just took it from example https://github.com/mailgun/mailgun-go/blob/master/examples/examples.go#L15-L16.

Creating domain and routes works now.

I also added sensitive to smtp_password field, so it does not leak on plan stage.

I also noticed, that smtp_password field trigger domain destroy, which is not great, especially, that mailgun-go supports password changing. Perhaps that should be implemented with update-in-place (maybe separated issue, but I don't know if there is a demand for that).

I think it can be reviewed now.

phillbaker commented 5 years ago

Sounds good!

On Wed, Apr 10, 2019 at 1:39 PM Mateusz Gozdek notifications@github.com wrote:

@invidian commented on this pull request.

In resource_mailgun_domain.go https://github.com/phillbaker/terraform-provider-mailgunv3/pull/7#discussion_r273795800 :

if err != nil {

return fmt.Errorf("Error deleting domain: %s", err) }

// Give the destroy a chance to take effect return resource.Retry(1time.Minute, func() resource.RetryError {

  • , , _, err = client.GetSingleDomain(d.Id())
  • ctx, cancel := context.WithTimeout(context.Background(), time.Second*30)
  • defer cancel()

It looks like just context.Background() should be used, examples:

- https://github.com/terraform-providers/terraform-provider-google/blob/1319ddd86de649e62984da8a31ca5643c4ef1787/google/resource_google_project_services.go#L209

https://github.com/terraform-providers/terraform-provider-github/blob/6ed5159cce57eee82a11546135579e0b508adf81/github/resource_github_membership.go#L48

So maybe I just change the code to that?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/phillbaker/terraform-provider-mailgunv3/pull/7#discussion_r273795800, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFxKSykCe8R_DZuDP9iFaD_wlZxVf_zks5vfXkYgaJpZM4cZH5H .

invidian commented 5 years ago

Updated with plain context.Background().