Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Allow users to upload notifications #120

Merged
merged 23 commits into from
Dec 21, 2022

Conversation

JonathanLorimer
Copy link
Contributor

@JonathanLorimer JonathanLorimer commented Dec 14, 2022

This PR adds support for the per-step notify key that buildkite accepts.

https://buildkite.com/docs/pipelines/notifications

@adikari
Copy link
Owner

adikari commented Dec 14, 2022

Can you provide details of what this PR is about and what problem it solves?

@JonathanLorimer
Copy link
Contributor Author

Can you provide details of what this PR is about and what problem it solves?

Yes, Its only in draft right now because I was just testing it. I need to cleanup a lot of stuff (as you can see I modified the hooks executable for easier testing).

The gist is that I want to be able to provide a custom notify clause in the monorepo-plugin config so that I can customize the github commit statuses https://buildkite.com/docs/integrations/github#customizing-commit-statuses. Right now the yaml marshalling doesn't support this key.

Copy link
Contributor Author

@JonathanLorimer JonathanLorimer left a comment

Choose a reason for hiding this comment

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

Self-review

plugin.go Outdated
@@ -8,7 +8,7 @@ import (
log "github.com/sirupsen/logrus"
)

const pluginName = "github.com/monebag/monorepo-diff"
const pluginName = "monorepo-diff-buildkite-plugin"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to revert this change along with the strings.Contains below, but this seemed to be more flexible to me. Let me know what you think.

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to leave it as it is. Once you revert it back I will merge the PR

@JonathanLorimer JonathanLorimer marked this pull request as ready for review December 15, 2022 14:43
@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #120 (640acd2) into master (c1ecdb9) will increase coverage by 3.13%.
The diff coverage is 95.65%.

❗ Current head 640acd2 differs from pull request most recent head 7eeaffa. Consider uploading reports for the commit 7eeaffa to get more accurate results

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   80.55%   83.68%   +3.13%     
==========================================
  Files           4        4              
  Lines         180      331     +151     
==========================================
+ Hits          145      277     +132     
- Misses         23       42      +19     
  Partials       12       12              
Impacted Files Coverage Δ
main.go 44.00% <0.00%> (-2.67%) ⬇️
util.go 93.54% <87.50%> (+0.21%) ⬆️
plugin.go 96.07% <100.00%> (+1.48%) ⬆️
pipeline.go 73.77% <0.00%> (+2.71%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

plugin.go Outdated
@@ -53,6 +53,7 @@ type Step struct {
Env map[string]string `yaml:"env,omitempty"`
Async bool `yaml:"async,omitempty"`
SoftFail interface{} `json:"soft_fail" yaml:"soft_fail,omitempty"`
Notify interface{} `yaml:"notify,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to add proper type with possible values? Will serve as a documentation and also validation.

@adikari adikari force-pushed the jonathan/upload-notify branch from 2e91bd2 to 245c747 Compare December 20, 2022 22:18
@adikari adikari merged commit d83cf9d into adikari:master Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants