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

fix(notification): Add application to githubStatus notification context #1106

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dbrougham
Copy link
Contributor

@dbrougham dbrougham commented Jun 3, 2021

Add the application to the github status notification. This helps ensure checks are unique per service when working in a monorepo.

This functionality is behind the feature flag hasApplicationNameInContext to ensure backwards compatibility.

Issue: spinnaker/spinnaker#6442

@dbrougham dbrougham force-pushed the dbrougham/feature-githubStatus/add-pipeline-to-context branch 4 times, most recently from 6d1aae7 to add13f6 Compare June 10, 2021 21:02
@@ -91,7 +93,7 @@ public void sendNotifications(
content.getStageIndex());
} else if (config.get("type").equals("pipeline")) {
description = String.format("Pipeline '%s' is %s", content.getPipeline(), status);
context = String.format("pipeline/%s", content.getPipeline());
context = String.format("%s/pipeline/%s", application, content.getPipeline());

Choose a reason for hiding this comment

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

I think this will be quite disruptive for existing users of the feature - if the existing context has been set to required in the github branch-protection settings, then this will leave github waiting for statuses that never match the required value. I think that if the change is made at the end instead, then github will still match it, as a prefix. i.e. change it to pipeline/$pipeline/$application

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

Yeah I did think that any repositories using this feature may be required to update their GitHub checks if they are set to required.

I couldn't seem to find anything online in the GitHub docs that mention a partial match working for the GitHub status. Will do some testing on this today.

Alternatively we could have a user specified context that is sent although this will require more code changes to implement, but would be backwards compatible, or we could look to use a feature flag to specify the full path for GitHub notification context

Copy link
Contributor Author

@dbrougham dbrougham Jun 21, 2021

Choose a reason for hiding this comment

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

After some testing it doesn't look like the GitHub status will behave that way
https://docs.github.com/en/rest/reference/repos#statuses

We could add this to the release notes as a potential breaking change to be aware of?

Two alternate ways forward:

  1. Put this functionality behind some kind of feature flag for echo detailedGitHubStatusNotification
  2. Allow users to specify a custom context in the notification pipeline config/deck UI

@dbrougham dbrougham marked this pull request as draft November 1, 2021 04:02
@dbrougham dbrougham force-pushed the dbrougham/feature-githubStatus/add-pipeline-to-context branch from 6f3f9ea to 632da97 Compare November 7, 2021 20:20
@dbrougham dbrougham closed this Nov 7, 2021
@dbrougham dbrougham force-pushed the dbrougham/feature-githubStatus/add-pipeline-to-context branch from 632da97 to 1acd51f Compare November 7, 2021 20:21
@dbrougham dbrougham reopened this Nov 7, 2021
@dbrougham dbrougham force-pushed the dbrougham/feature-githubStatus/add-pipeline-to-context branch from 3655f00 to a4a5123 Compare November 7, 2021 20:24
@dbrougham dbrougham force-pushed the dbrougham/feature-githubStatus/add-pipeline-to-context branch from a4a5123 to d143845 Compare November 7, 2021 20:24
@dbrougham dbrougham marked this pull request as ready for review November 7, 2021 20:29
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.

2 participants