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

Refer to smtp_config by ID #185

Closed
wants to merge 2 commits into from

Conversation

cimnine
Copy link

@cimnine cimnine commented Jun 26, 2024

This updates the terraform provider to refer to the SMTP Configuration by their IDs. This is a breaking change! Previous SMTP configs need to be removed from the state and re-imported with their proper resource ID.

Fixes #180

Todo

  • Fix import acceptance test

Definition of Ready

  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • No debug or dead code
  • My code has no repetitions
  • All non-functional requirements are met
  • The generic lifecycle acceptance test passes for affected resources.
  • Examples are up-to-date and meaningful. The provider version is incremented.
  • Docs are generated.
  • Code is generated where possible.

- Update Zitadel provider
- Update the smtp_config resource to work with the ID
@cimnine cimnine changed the title SMTP Config By ID Refer to smtp_config by ID Jun 26, 2024
@cimnine
Copy link
Author

cimnine commented Jun 26, 2024

The test currently fails as follows:

=== RUN   TestAccSMTPConfig
=== PAUSE TestAccSMTPConfig
=== CONT  TestAccSMTPConfig
    lifecyletest.go:73: Step 5/7 error running import: exit status 1
        
        Error: Cannot import non-existent remote object
        
        While attempting to import an existing object to
        "zitadel_smtp_config.default", the provider detected that no object exists
        with the given id. Only pre-existing objects can be imported; check that the
        id is correct and that it is associated with the provider's configured region
        or endpoint, or use "terraform apply" to create a new remote object for this
        resource.
        
--- FAIL: TestAccSMTPConfig (4.62s)

I'm not sure how to proceed. There is probably no default SMTP config anymore, so to import a default SMTP config, one would have to be created beforehand. Is this the correct assumption and how to do that?

@cimnine
Copy link
Author

cimnine commented Jun 28, 2024

For reference: Via Discord Migual A. C suggested that this resource should also activate the SMTP configuration:

For your code I'd add the followint to the smtpconfig/funcs.go to activate the provider after it has been created:

    _, err = client.ActivateSMTPConfig(ctx, &admin.ActivateSMTPConfigRequest{Id: resp.Id})
    if err != nil {
        return diag.Errorf("failed to activate smtp config: %v", err)
    }

However, I don't believe that this is a good idea:

I believe the activate part should be a new independent resource.
Otherwise, if there are multiple parallel configurations, it will never be deterministic which configuration is currently active.

There could also be an is_active flag on the zitadel_smtp_config resource. But it would be hard to enforce that only one zitadel_smtp_config defines is_active = true:

The benefit of an independent resource compared to an is_active flag (or similar) is, that with the resource we can use the internal terraform ID to ensure that there is only one zitadel_active_smtp_config (or whatever the name shall be).

@eliobischof eliobischof requested review from stebenz and removed request for stebenz and eliobischof August 12, 2024 08:36
@stebenz
Copy link
Contributor

stebenz commented Aug 29, 2024

HI @cimnine, I took the liberty and included this changes in the new v2.0.0 release, as it was necessary to include it to upgrade zitadel/zitadel-go dependency to v3.
I will close this as it is still a draft, can you check if your problem is solved with the new verison? The SMTP config has be imported again unfortunately.

@cimnine
Copy link
Author

cimnine commented Aug 30, 2024

I can't check, as I run into #195 when trying.

@cimnine
Copy link
Author

cimnine commented Sep 5, 2024

@stebenz I believe the resource's documentation is wrong for the import:

# The resource can be imported using the ID format `<id[:password]>`, e.g.
terraform import zitadel_smtp_config.imported '123456789012345678:p4ssw0rd'

But other than that, it works for me.

@cimnine cimnine closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update to zitadel_smtp_config fails
3 participants