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

[tf-module:push-notifications] Allow to define multiple apps per client platform #347

Merged
merged 3 commits into from
Oct 9, 2020

Conversation

lucendio
Copy link
Contributor

@lucendio lucendio commented Oct 2, 2020

NOTE: This is a breaking change and requires Terraform state migration ('terraform state mv')

Due to the fact that multiple different clients (app/build ID) can be connected to
the same backend and at the same time desire to receive push notifications, it should
be possible to register those apps to the same events/notifications queue.

preliminary work for: https://github.com/zinfra/backend-issues/issues/1794

…ent platform

NOTE: This is a breaking change and requires Terraform
      state migration ('terraform state mv')

Due to the fact that multiple different clients (app/build ID) can be connected to
the same backend and at the same time desire to receive push notifications, it should
be possible to register those apps to the same events/notifications queue.
@lucendio lucendio changed the title [tf-module:push-notifications] Allow to defined multiple apps per client platform [tf-module:push-notifications] Allow to define multiple apps per client platform Oct 5, 2020
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

requires Terraform state migration ('terraform state mv')

Could you document the state migration procedure required in a bit more detail?

@lucendio
Copy link
Contributor Author

lucendio commented Oct 5, 2020

requires Terraform state migration ('terraform state mv')

Could you document the state migration procedure required in a bit more detail?

Sure :+1

@lucendio
Copy link
Contributor Author

lucendio commented Oct 5, 2020

Additionally, @akshaymankar feedback was:

  1. Why do we have two platform resources - one for ios and one for android, if they are so little different?

After giving this a little bit of thought, I came to the conclusion, that it would have little value to move them together. The arguments are:

  • TF still does not support optional fields in a (nested) object. Without that, it wouldnt make much sense - type-wise - to move ios_applications and android_applications together into applications.
  • the code contains some information around the differences between both and points to further documentation. which is helpful if one starts looking into this starting with the code
  • (opinion!) overall, I feel it is more readable and I don't mind a little bit of copy&paste in infa code.
  • NOTE: I didnt verify whether the AWS API (and Terraform) could deal with platform_principal being set to null in case of Android
  1. Dont use a list, use a map and the app ID as key

I had another peek into how our own SNS setup currently looks like. The name of aws_sns_platform_application indeed shows up multiple times, but always with a different platform. So, a map of app IDs as key is not an option in my understanding, especially in cases where platforms may vary in their credentials. Due to this, I will allow the platform to be an array and therefore need to rework the for_each to incorporate the platform in order to make the resource ID.

@lucendio lucendio force-pushed the tf-module-push-notifications-multi-app-support branch from 939f568 to b700483 Compare October 8, 2020 20:31
…rces

This changeset unifies both platform resources. This is possible because
TF - for once - behaves as expected and drops a resource attribute that
is set to 'null'.
This also helps to deduplicate the logic that is used to massage the app
lists inputs to cope with 'for_each'.

Allow the list of apps to not being set. Because this is now allowed, the
overall existence of any application is used as indicator to figure out
whether the IAM policy has to be created or not.
@lucendio lucendio force-pushed the tf-module-push-notifications-multi-app-support branch from b700483 to 41043bd Compare October 9, 2020 11:02
@arianvp arianvp self-requested a review October 9, 2020 12:35
@lucendio lucendio merged commit 6dcf00f into develop Oct 9, 2020
@lucendio lucendio deleted the tf-module-push-notifications-multi-app-support branch October 9, 2020 12:42
@fisx fisx mentioned this pull request Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants