argoproj / notifications-engine

Affordable notifications for Kubernetes
Apache License 2.0
271 stars 147 forks source link

github: reuse existing deployment when sending new notification #235

Closed MrFreezeex closed 8 months ago

MrFreezeex commented 11 months ago

Try to list existing deployment with the same sha/environment/ref and reuse it if found. This allows ArgoCD notification to update the same deployment with multiple deployment status instead of creating a new one on each deployment notification.

codecov[bot] commented 11 months ago

Codecov Report

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

Comparison is base (2daee60) 55.05% compared to head (f523570) 54.79%.

:exclamation: Current head f523570 differs from pull request most recent head be7b188. Consider uploading reports for the commit be7b188 to get more accurate results

Files Patch % Lines
pkg/services/github.go 0.00% 23 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #235 +/- ## ========================================== - Coverage 55.05% 54.79% -0.27% ========================================== Files 35 35 Lines 3360 3376 +16 ========================================== Hits 1850 1850 - Misses 1238 1254 +16 Partials 272 272 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MrFreezeex commented 11 months ago

Hi @jaredtbates I saw that you mentioned in your Initial pull request that you didn't find a way to preserve deployment id, what do you think about this patch?

pasha-codefresh commented 8 months ago

@MrFreezeex could you please resolve conflicts and make sure that you have added tests?

MrFreezeex commented 8 months ago

@MrFreezeex could you please resolve conflicts and make sure that you have added tests?

Hi @pasha-codefresh! I just rebased the code but I can't realistically add test unfortunately. This whole function is not tested and AFAIK there are no tests on any Send method anywhere. The tests are up until the actual Send is done but then it's always not covered by tests.

pasha-codefresh commented 8 months ago

LGTM, thank you