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

github: reuse existing deployment when sending new notification #235

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

MrFreezeex
Copy link
Contributor

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.

@MrFreezeex MrFreezeex force-pushed the github-reuse-deployment branch 2 times, most recently from 538dbb2 to bbbd023 Compare October 19, 2023 17:29
@codecov
Copy link

codecov bot commented Oct 19, 2023

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%.

❗ 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 ⚠️
Additional details and impacted files
@@            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              

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

@MrFreezeex
Copy link
Contributor Author

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?

Copy link
Contributor

@jaredtbates jaredtbates left a comment

Choose a reason for hiding this comment

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

This looks awesome to me! This is a smart solution.

@pasha-codefresh
Copy link
Member

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

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.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@MrFreezeex
Copy link
Contributor Author

MrFreezeex commented Feb 1, 2024

@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
Copy link
Member

LGTM, thank you

@pasha-codefresh pasha-codefresh merged commit c0913e2 into argoproj:master Feb 7, 2024
3 checks passed
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