-
Notifications
You must be signed in to change notification settings - Fork 141
feat: GitHub App integration #180
feat: GitHub App integration #180
Conversation
Codecov Report
@@ Coverage Diff @@
## master #180 +/- ##
==========================================
- Coverage 52.69% 51.53% -1.16%
==========================================
Files 33 34 +1
Lines 1577 1690 +113
==========================================
+ Hits 831 871 +40
- Misses 586 647 +61
- Partials 160 172 +12
Continue to review full report at Codecov.
|
18631f1
to
4187b4e
Compare
ec141d7
to
4b2ee4a
Compare
@ryota-sakamoto the PR looks awesome! It makes integration with Github much easier. Thank you! |
I think so too, it is very easy. I'm finished implementation and test, could you review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the review took some time since I was on a short vacation.
PR looks great. Added one proposal to allow extending github integration in the future.
@@ -60,6 +60,34 @@ func replaceStringSecret(val string, secretValues map[string][]byte) string { | |||
}) | |||
} | |||
|
|||
func replaceServiceConfigSecret(data map[string]interface{}, secretValues map[string][]byte) map[string]interface{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change required to make replacement more accurate and avoid accidentally produce invalid YAML?
The change looks good to me, just want to understand why it was required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right.
Currently, argocd-notifications-secret
is not supported to set multiline value such as private key.
If the user set multiline value, invalid data format of yaml will be created.
template.app-deployed: | | ||
message: | | ||
Application {{.app.metadata.name}} is now running new version of deployments manifests. | ||
github: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to leave room for additional actions such as create commit comment or create pull request (e.g. to create PR that rollback commit if app degrades). To allow adding actions we just need to move state
, label
and targetURL
fields under status
field:
github:
status:
state: success
label: "continuous-delivery/{{.app.metadata.name}}"
targetURL: "{{.context.argocdUrl}}/applications/{{.app.metadata.name}}?operation=true"
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good idea.
It can be easily added more actions and github.status
is more correctly than github
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
Hi @alexmt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thank you @ryota-sakamoto !
refs: #99
Remaining work: