Restore deprecated NotificationsConfig.Region field

[andrewwdye]

Tracking issue

Fixes #3070
Fixes #4296

Describe your changes

This change

• Restores population of the NotificationConfig.Region field from the flyte-core configmap template. This was removed in fix helm notifications config #4055. While the config struct field has a comment indicating deprecated, the alternative NotificationConfig.AWSConfig.Region is not consumed everywhere yet. It is not safe to stop populating this field yet, and is in fact required for all existing versions of flyteadmin to use NotificationsProcessor and Emailer. The deprecated field is only populated in the absence of the aws block.
• Updates flyteadmin to consume NotificationConfig.AWSConfig.Region and only fallback to the deprecated field if necessary.

Testing

I ran through a couple EKS and GCP helm templating scenarios to verify the fix correctly populates the deprecated region field if used and no other changes otherwise.

EKS with deprecated region field

❯ cat values-eks-notifications-deprecated.yaml
workflow_notifications:
  enabled: true
  config:
    notifications:
      type: aws
      region: "{{ .Values.userSettings.accountRegion }}"

Before:

❯ helm template flyte-core -f flyte-core/values-eks.yaml -f values-eks-notifications-deprecated.yaml | grep notifications.yaml -A3
  notifications.yaml: |
    notifications:
      type: aws
      publisher:

After:

❯ helm template flyte-core -f flyte-core/values-eks.yaml -f values-eks-notifications-deprecated.yaml | grep notifications.yaml -A4
  notifications.yaml: |
    notifications:
      type: aws
      region: <AWS_REGION>
      publisher:

❯ diff generated-old-eks-deprecated.yaml generated-new-eks-deprecated.yaml
3a4
>       region: <AWS_REGION>

EKS with AWS region field

❯ helm template flyte-core -f flyte-core/values-eks.yaml -f values-eks-notifications.yaml | grep notifications.yaml -A14 > generated-old-eks.yaml

❯ helm template flyte-core -f flyte-core/values-eks.yaml -f values-eks-notifications.yaml | grep notifications.yaml -A15 > generated-new-eks.yaml

❯ diff generated-old-eks.yaml generated-new-eks.yaml

GCP

❯ cat values-gcp-notifications.yaml
workflow_notifications:
  enabled: true
  config:
    notifications:
      type: gcp
      gcp:
        projectId: foo
      publisher:
        topicName: "arn:aws:sns:{{ .Values.userSettings.accountRegion }}:{{ .Values.userSettings.accountNumber }}:flyte-notifications-topic"
      processor:
        queueName: flyte-notifications-queue
        accountId: "{{ .Values.userSettings.accountNumber }}"
      emailer:
        subject: "Flyte: {{ project }}/{{ domain }}/{{ launch_plan.name }} has {{ phase }}"
        sender: "[email protected]"
        body: |
         "Execution {{ workflow.project }}/{{ workflow.domain }}/{{ workflow.name }}/{{ name }} has {{ phase }}.
          Details: https://flyte.example.com/console/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}.
          {{ error }}"

❯ helm template flyte-core -f flyte-core/values-gcp.yaml -f values-gcp-notifications.yaml | grep notifications.yaml -A14 > generated-old-gcp.yaml

❯ helm template flyte-core -f flyte-core/values-gcp.yaml -f values-gcp-notifications.yaml | grep notifications.yaml -A14 > generated-new-gcp.yaml

❯ diff generated-old-gcp.yaml generated-new-gcp.yaml

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

[codecov]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (4fc4988) 58.73% compared to head (50c0245) 59.28%.
Report is 4 commits behind head on master.

❗ Current head 50c0245 differs from pull request most recent head 20bb679. Consider uploading reports for the commit 20bb679 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4299      +/-   ##
==========================================
+ Coverage   58.73%   59.28%   +0.54%     
==========================================
  Files         583      544      -39     
  Lines       50331    38933   -11398     
==========================================
- Hits        29562    23081    -6481     
+ Misses      18396    13572    -4824     
+ Partials     2373     2280      -93     

Flag	Coverage Δ
unittests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
flyteadmin/pkg/async/notifications/factory.go	18.35% <0.00%> (-0.21%)	⬇️

... and 554 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[eapolinario]

Thank you.