GitHub Deployment - Allow Automerge configuration

[mikesplain]

Hi there, thanks for adding support for Deployments to the Notifications Engine. I was testing out the support and ran into issues trying to deploy an older ref from the default branch. The notifications controller would report an error when merging the default branch into the commit referenced. Upon reading the docs, it looks like this is the intended behavior so it seems reasonable to allow users to disable this feature when they would not like their deployment updated against the default branch.

This PR was tested against an internal custom ArgoCD build and it resolved our issues with deployments only functioning on the latest commit in a default branch.

I'm happy to make any required changes, just let me know. Thanks!

[codecov]

Codecov Report

Patch coverage: 28.57% and project coverage change: -0.06% ⚠️

Comparison is base (8570c23) 54.31% compared to head (898778a) 54.25%.
Report is 1 commits behind head on master.

❗ Current head 898778a differs from pull request most recent head 2c52675. Consider uploading reports for the commit 2c52675 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
- Coverage   54.31%   54.25%   -0.06%     
==========================================
  Files          35       35              
  Lines        3198     3205       +7     
==========================================
+ Hits         1737     1739       +2     
- Misses       1208     1211       +3     
- Partials      253      255       +2     

Files Changed	Coverage Δ
pkg/services/github.go	34.80% <28.57%> (-0.20%)	⬇️

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

[mikesplain]

Please hold off on this at the moment, I want to do another round of testing now that I got the tests and linter to pass.

[pasha-codefresh]

@mikesplain is it ready for review?

[mikesplain]

@pasha-codefresh Yes It is, thank you! Just finished a second round of testing.

[pasha-codefresh]

Can we also mention that this parameter is optional and by default it is false, i would write opposite , like: Setting this option to true is required ....

[mikesplain]

Sorry for the delay I realized that you're right, this PR set the value to false by default however the upstream API is true by default. I had to modify the PR and language in the doc to reverse the behavior. I hope the language in the docs is more clear now. Thanks!

[pasha-codefresh]

@mikesplain LGTM, just small comment about documentation

[carloscastrojumo]

I was just looking for this feature @mikesplain . Thanks for the PR. Are you still available to address the issues requested by the reviewers?

[mikesplain]

Yes, found a bug and had to fix the code slightly. Tested it internally and it works fine just need to rebase. I'll update the PR tomorrow ASAP.

[mikesplain]

@pasha-codefresh I apologize for the long delay, life gets in the way. This is ready for review again and has been rebased against master!

After your comments I realized the PR worked properly if you set the new configuration however the default was false, rather than the Github default (and argocd notifications default, true), see https://docs.github.com/en/rest/deployments/deployments?apiVersion=2022-11-28#create-a-deployment for more details.

I switched the use of pointers so we can check if the user has set the configuration and default it accordingly. I'm not sure if the project has a better way to handle this, I'm happy to amend with any suggestions. Thanks again

[carloscastrojumo]

Any news on this @pasha-codefresh ?

[pasha-codefresh]

Sorry, hadnt time to review, @carloscastrojumo @mikesplain , i will do it today / tomorrow

[pasha-codefresh]

LGTM, thank you

[mikesplain]

Great thanks so much @pasha-codefresh!

[pasha-codefresh]

Thank you for great job :) you can open PR to argo-cd with notification-engine update, you can send it here, i will help with review