Skip to content

pr_notifier: Add a link to the github instructions for disabling actions to the PR reminders script.#18565

Closed
RyanTheOptimist wants to merge 4 commits intoenvoyproxy:mainfrom
RyanTheOptimist:ReminderActions
Closed

pr_notifier: Add a link to the github instructions for disabling actions to the PR reminders script.#18565
RyanTheOptimist wants to merge 4 commits intoenvoyproxy:mainfrom
RyanTheOptimist:ReminderActions

Conversation

@RyanTheOptimist
Copy link
Copy Markdown
Contributor

Add a link to the github instructions for disabling actions to the PR reminders script.

Risk Level: Low - Tools only
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

@RyanTheOptimist
Copy link
Copy Markdown
Contributor Author

/assign @alyssawilk

PR reminders script.

Signed-off-by: Ryan Hamilton <rch@google.com>
@alyssawilk
Copy link
Copy Markdown
Contributor

Alas presubmits :-P
/wait

Signed-off-by: Ryan Hamilton <rch@google.com>
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement!

That is a ridiculous link. I'm torn because I prefer the shortlink form but I don't think I'd let people I hadn't worked with for years use bitly since you can chance the underlying link. Calling in @phlax or a second opinion for if we should list the full glorioys

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository

@RyanTheOptimist
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18565 (comment) was created by @RyanTheOptimist.

see: more, trace.

@phlax
Copy link
Copy Markdown
Member

phlax commented Oct 12, 2021

mho is that if we want to include this (not quite getting the rationale to include it here) then we definitely need to include the full link

it may be painfully long, but at least it is semantically meaningful

Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist
Copy link
Copy Markdown
Contributor Author

Replaced the bit.ly URL with the full github URL.

alyssawilk
alyssawilk previously approved these changes Oct 12, 2021
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

LGTM pending CI :-)

Signed-off-by: Ryan Hamilton <rch@google.com>
if not SLACK_BOT_TOKEN:
print(
'Missing SLACK_BOT_TOKEN: please export token from https://api.slack.com/apps/A023NPQQ33K/oauth?'
' or disable actions via the instructions here: '
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alternatively, it would be less work to fail open and have this action succeed with this message if SLACK_BOT_TOKEN is missing. Is the assumption that the majority of Envoy contributors have their SLACK_BOT_TOKEN set up?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did consider that. The only reason I didn't do that is then if the token were missing in the main repo where this runs, it would silently fail to send out notifications and that might complicate debugging? But if this is really happening to everyone then maybe that's the right tradeoff?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this action not just bail if its not the envoy repo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I should have mentioned that I investigated that approach first. But from the docs I could not figure out a way to run just on the main repo. :/ Do you happen to know?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alyssawilk
Copy link
Copy Markdown
Contributor

I think the right way to disable would probably be to adjust the repo syntax to only run for the envoyproxy/envoy repo, rather than have it run but fail open.
it's still a lot of work to do the bits that go before that.

@RyanTheOptimist
Copy link
Copy Markdown
Contributor Author

I think I will abandon this PR in favor of #18592. Thanks for the help!

@RyanTheOptimist RyanTheOptimist marked this pull request as draft October 12, 2021 20:39
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.

4 participants