feat(extensions): add Discord extension and additional metadata #177
Conversation
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
|
|
||
| // Merge attachment and integration configs | ||
| // If there are not attachment configuration then create an empty map | ||
| if attachment.Config == nil { |
There was a problem hiding this comment.
I noticed that this code was buggy when the registration didn't have any metadata.
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
migmartri
changed the title
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
migmartri
changed the title
migmartri
changed the title
| wf := attachment.Workflow | ||
| integration := attachment.Integration | ||
|
|
||
| // Merge attachment and integration configs |
There was a problem hiding this comment.
Are we merging integration and attachment configs or we are merging configurations provided during registration and attachment?
There was a problem hiding this comment.
Hi.
Not sure I understand the difference, but we are showing together both registration and attachment configuration states. Does it make sense?
There was a problem hiding this comment.
Yeah, I am nitpicking here a bit, but I was not sure at first what you mean by the integration config here.
| continue | ||
| } | ||
| options = append(options, fmt.Sprintf("%s: %v", k, v)) | ||
| maps.Copy(attachment.Config, integration.Config) |
There was a problem hiding this comment.
I think this is not correct. We are overwriting the configuration provided at the attachment state, and I think we should either show both (attachment and registration configs) or use the attachment value instead in case the keys are the same.
There was a problem hiding this comment.
To give you an example, imagine I want to add another To field at the attachment stage in the smtp extension because today the to field in the registration is more like a default value and I may want to have different values for different workflows (set via attachment).
--- a/app/controlplane/extensions/core/smtp/v1/extension.go
+++ b/app/controlplane/extensions/core/smtp/v1/extension.go
@@ -49,10 +49,12 @@ type registrationState struct {
}
type attachmentRequest struct {
+ To string `json:"to,omitempty" jsonschema:"format=email,description=The email address to send the ema
il to."`
CC string `json:"cc,omitempty" jsonschema:"format=email,description=The email address of the carbon c
opy recipient."`
}
type attachmentState struct {
+ To string `json:"to"`
Does it make sense?
There was a problem hiding this comment.
Yeah, that's a good point. I updated it to instead use the attachment config in the case of a conflict.
Thanks!
|
|
||
| ```console | ||
| $ chainloop integration registered add discord-webhook --opt webhook=[webhookURL] | ||
| ``` |
There was a problem hiding this comment.
Do you think we should mention anything about the way we store webhooks? We treat them like credentials, which may be worth mentioning here.
There was a problem hiding this comment.
as an user I don't see value on mentioning it, or do you think that it could help with adoption?
There was a problem hiding this comment.
ok. I would like to know that this is stored securely without looking into the code.
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Extension that leverages Discord webhooks to send attestations.
The inputs at registration time are a required webhook URL and an optional username override
The result is a message that includes some metadata in the content body and an attached intoto attestation.
This patch also includes some improvements in the sdk. we now inject additional workflow information (name, project) as well as the runnerURL. refs #174 cc/ @danlishka
That new data is used to craft the resulting message.
Another interesting aspect about the implementation, is that during registration I fetch information about the webhook itself (i.e name set in discord) so we can clearly differentiate them.
Closes #176