Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Implement Sandbox notifications processor and publisher #595

Merged

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Jul 27, 2023

TL;DR

Implement notifications processor and publisher for Sandbox.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Add new SandboxProcessor and SandboxPublisher that implement the Processor interface.

I add config in yaml file like this

notifications:
  type: sandbox
  emailer:
    emailServerConfig:
      apiKeyEnvVar: "SENDGRID_API_KEY"
    subject: "Notice: Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\"."
    sender: "[email protected]"  # Consider using a generic email instead of personal
    body: >
      Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\". View details at
      <a href=\http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}>
      http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}</a>. {{ error }}

screenshot
image

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Future Outlier and others added 7 commits July 27, 2023 22:42
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Flyte-Bot <[email protected]>
Co-authored-by: eapolinario <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Flyte-Bot <[email protected]>
Co-authored-by: flyte-bot <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Future Outlier added 3 commits July 28, 2023 21:43
@codecov
Copy link

codecov bot commented Jul 29, 2023

Codecov Report

Merging #595 (3721b9c) into master (04bcb15) will increase coverage by 1.84%.
The diff coverage is 100.00%.

❗ Current head 3721b9c differs from pull request most recent head 19aef7f. Consider uploading reports for the commit 19aef7f to get more accurate results

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage   58.42%   60.26%   +1.84%     
==========================================
  Files         168      170       +2     
  Lines       16393    13433    -2960     
==========================================
- Hits         9578     8096    -1482     
+ Misses       5964     4486    -1478     
  Partials      851      851              
Flag Coverage Δ
unittests ?

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

Files Changed Coverage Δ
pkg/async/notifications/factory.go 19.07% <100.00%> (+14.62%) ⬆️
...notifications/implementations/sandbox_processor.go 100.00% <100.00%> (ø)
...notifications/implementations/sandbox_publisher.go 100.00% <100.00%> (ø)

... and 154 files with indirect coverage changes

…dbox_processor_test.go

Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier
Copy link
Member Author

@pingsutw I've updated my test, please check it.
Also to use the mock emailer int test function, I've added a "return nil" in my sandbox_processor.go.
Thanks a lot.

@Future-Outlier
Copy link
Member Author

Future-Outlier commented Jul 30, 2023

I am not pretty sure whether use a noop emailer or a mock emailer, please get in touch with me if it is necessary to change the code.
Thanks a lot.

Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier
Copy link
Member Author

BTW, I just copy the master branch's go.mod and go.sum to my branch currently and run "go mod tidy"
I found that it is not needed to update the go.mod and go.sum.
Thanks a lot

pingsutw
pingsutw previously approved these changes Aug 6, 2023
katrogan
katrogan previously approved these changes Aug 8, 2023
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

Looks great. This is failing the code coverage check, but I think the code cov annotations have suggestions for some additional test cases we could use to bump the code coverage on this PR and so that we can test more scenarios

@Future-Outlier
Copy link
Member Author

Looks great. This is failing the code coverage check, but I think the code cov annotations have suggestions for some additional test cases we could use to bump the code coverage on this PR and so that we can test more scenarios

Thanks for the suggestions, I've learned a lot!

@Future-Outlier Future-Outlier dismissed stale reviews from katrogan and pingsutw via c2a9301 August 9, 2023 03:01
pingsutw
pingsutw previously approved these changes Aug 9, 2023
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

Thank you for adding all the unit tests!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants