Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add option to pass additional headers via secretRef to the generic notification provider #291

Closed
wants to merge 2 commits into from
Closed

Conversation

boxcee
Copy link
Contributor

@boxcee boxcee commented Nov 23, 2021

The generic notification provider should have a little more configuration options.

We have a use-case where we want to kick-off Github workflows based on deployments. A feature to add additional headers would allow us to do this. Without additional headers we are not able to authenticate with Github though, since it requires an Authorization header with the personal access token set.

@boxcee boxcee marked this pull request as ready for review November 23, 2021 19:38
@yebyen
Copy link

yebyen commented Nov 23, 2021

Hi @boxcee ! Thanks for your contribution here,

Can you please update the PR description with some explanation about what is added here? When you are ready to have it reviewed, can you please make sure the DCO bot is satisfied, and that you have squashed and/or --sign-offed your commits?

@boxcee boxcee changed the title options-for-generic-webhook Feature: Add options to pass additional headers via secretRef to the generic notification provider Nov 23, 2021
@boxcee boxcee changed the title Feature: Add options to pass additional headers via secretRef to the generic notification provider Feature: Add option to pass additional headers via secretRef to the generic notification provider Nov 23, 2021
Signed-off-by: Moritz Schmitz von Hülst <moritz@hauptstadtoffice.com>
@boxcee
Copy link
Contributor Author

boxcee commented Nov 23, 2021

@yebyen done 👍🏾

@modalkonform
Copy link

@yebyen
Is there any chance this change will get merged soon?

@@ -32,6 +32,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/predicate"

"gopkg.in/yaml.v3"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this dependency and use sigs.k8s.io/yaml

metadata:
name: generic-secret
namespace: default
data:
Copy link
Member

Choose a reason for hiding this comment

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

Please use stringData e.g.

stringData:
  headers: |
     Authorization: token
     X-Forwarded-Proto: https

Also explain how the headers are used.

Signed-off-by: Moritz Schmitz von Hülst <moritz@hauptstadtoffice.com>
@kingdonb
Copy link
Member

Can you provide a bit more detail about the example of use for documentation? I understand the sample and the change, I'm just unsure about how the GitHub side of the configuration would look like, and how Flux users should discover this feature.

The example added in the doc isn't given any context or description in the current revision of the PR. That would make this finished, afaict 👍 @boxcee the branch has merge conflicts right now and so, looks like it must be rebased in order to merge.

@stefanprodan
Copy link
Member

Superseded by #317

@alekspog
Copy link
Contributor

@boxcee hi, how do you trigger Github workflows with /dispatches URL with generic notification provider? It seems that Github repository_dispatch trigger requires a mandatory type_event field https://docs.github.com/en/rest/reference/repos#create-a-repository-dispatch-event, but I don't see how to pass custom fields with this generic provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants