[RFC] Dedicated API for commit status updates #2639
Conversation
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
|
@somtochiama I would suggest looking at this discussion. I kind of dropped the ball on implementing this last summer... |
|
@phillebaba I have! It was a very good starting point. |
There was a problem hiding this comment.
I think this RFC needs to address the multi-cluster issue that users are running into. @somtochiama have you've seen fluxcd/notification-controller#340?
|
|
||
| - [Implement GitHub commit status notifier](https://github.com/fluxcd/notification-controller/pull/27) | ||
| - [Add Gitlab notifier](https://github.com/fluxcd/notification-controller/pull/43) | ||
| - [Add bitbucket notifier](https://github.com/fluxcd/notification-controller/pull/73) |
There was a problem hiding this comment.
Azure DevOps is missing from here.
| eventSeverity: info | ||
| eventSources: | ||
| - kind: Kustomization | ||
| name: webapp |
There was a problem hiding this comment.
We should probably have a summary field here similar to Alert. This can be helpful in adding some metadata about the event, which can be helpful, for example when there are multiple clusters. (ref: fluxcd/notification-controller#194)
There was a problem hiding this comment.
The name summary doesn't make sense for commit status IMO, it should be prefix or something else.
|
@phillebaba @somtochiama does it make sense to add templating to CommitStatus if the actual message is limited to a couple of characters ? Maybe adding |
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
|
It would be good to take into account that cdevents is shaping. |
|
@somtochiama we need to take into account things like fluxcd/notification-controller#369 it's not a commit status but it's not an alert either... |
|
So I have looked this over now and for the most part this looks pretty straight forward. My opinion is that this should only cover CommitStatus as it seems to be a well defined term within most Git providers today. Everything else like Github Workflow dispatch should be covered in another RFC, as it would be to much work dealing with that too. I would like to push for adding some sort of templating feature to the commit status. Just because there would be so many different needs in the future that we cant really consider. The reasoning for why to have it in the first place is that we want to differ a commit status coming from different environments, but we could consider for example a environment with multiple regions. Does just having a prefix solve this? I am not sure but maybe. One use case which my comment covered was the option to add templating parameters from a configmap which would enable the same commit status to be deployed into multiple clsuters but result in different prefixes. The question is mostly how much of an hassle that work is worth. |
Agreed, I've merged the GitHub dispatch integration for the Alert API. |
There was a problem hiding this comment.
First round of comments to steer towards something I would be comfortable with.
In general, I think the document needs more tidying in terms of style, line wraps, punctuation, etc. which I didn't nitpick on in full.
Other than this, I think what the RFC itself is suggesting is the right direction.
|
|
||
| ## Summary | ||
|
|
||
| There should be a dedicated kind in the notification controller for sending commit status notifications to git providers. |
There was a problem hiding this comment.
I would expect the summary to be written as [This RFC] <summary>. So in this particular case, something like:
This RFC proposes to create a dedicated
CommitStatuskind in the to allow configuration of commit status notifications to a (Git)Provider.
|
|
||
| ## Motivation | ||
|
|
||
| Currently, The Alert type can reference both git providers and chat providers. However, the differences between the two providers and how notifications being sent to them should be handled has continued to diverge. |
There was a problem hiding this comment.
Currently, the ...- git -> Git
- I would provide examples of what Git providers and/or chat providers are to provide further context.
- The last part of this line is difficult to read ("being sent to them should be handled has continued to diverge").
| ## Motivation | ||
|
|
||
| Currently, The Alert type can reference both git providers and chat providers. However, the differences between the two providers and how notifications being sent to them should be handled has continued to diverge. | ||
| For example, there is a limit on the length of the name/context of the status for git provider. |
| When one commit triggers more than one deployment/reconciliation in different clusters, the commit status notification from one overwrites the other as seen in this [issue](https://github.com/fluxcd/notification-controller/issues/195). | ||
| A new field specific to the git commit status will allow Flux users to specify a prefix that will be prepended to the context to differentiate the statuses. | ||
|
|
||
| By creating a separate kind for git commit status, the specific nuances of the provider can be properly taken into account and it would allow more flexibility in adding fields specific to each kind. |
There was a problem hiding this comment.
I would restructure the argumentation here to gradually get to the point of introducing a new kind, because the changes to the current kind or not justified.
Paraphrasing:
We have observed the issue to be <problem>, this would require the introduction of
<something> because of <reason>. Which is ultimately solved by <introducing new kind>,
because of <argumentation>.
|
|
||
| ## Implementation History | ||
|
|
||
| - [Implement GitHub commit status notifier](https://github.com/fluxcd/notification-controller/pull/27) |
There was a problem hiding this comment.
I would think none of these entries belong under "implementation history", as they are not related to this RFC but rather to what moved us to creating this RFC. Making it e.g. part of the motivation, which is the section in which you provide historical context to build up your case.
| Introduce a new kind in the notification-controller called `CommitStatus`. | ||
| The `CommitStatus` kind will similar to the `Alert` kind, referencing a provider and including event sources to accept events from. The major difference is that it will only reference git providers. Additionally, it can only have a `Kustomization` as its event source since it requires the `revision` field in the event metadata. | ||
| It would include a new field `.spec.prefix` that will be used to differentiate results of multiple deployments on different clusters triggered by the same commit and prevent one from overwriting the other. | ||
|
|
||
| This proposal will also introduce breaking changes as the `Alert` kind will no longer send notifications to git providers. |
There was a problem hiding this comment.
This is very thin and lacks technical details.
|
|
||
| ### Alternatives | ||
|
|
||
| Alternatively, we could keep using the `Alert` kind for sending notifications to git providers. While this might not be much of a pain now, it would continue to grow as the implementations details of new features might differ for the chat and git providers. |
There was a problem hiding this comment.
Instead of
While this might not be much of a pain now, ...
I would phrase it as (something like):
This would not allow for further growth of the Git and chat providers. As the implementation details of new features might differ, making it difficult to invent an API around this which is self explanatory and user friendly.
Proposal for adding a new API kind
CommitStatusto the notification-controller.Signed-off-by: Somtochi Onyekwere somtochionyekwere@gmail.com