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 slack reminder errors when running locally #333

Merged
merged 1 commit into from
May 3, 2024

Conversation

panentheos
Copy link
Collaborator

This fixes the slack reminder logic to correctly suppress reminders when running locally with no SLACK_WEBHOOK_URL environment variable set. Note that the default value in Application.get_env was not doing anything, since there's always a value present from config/runtime.exs, even if it's nil.

@digitalcora
Copy link
Contributor

Interesting, I didn't know Screenplay did this. I notice the module doc refers to #ctd-pio-livepa, which appears to have been archived in favor of a private channel I don't have access to. Do you know where this goes now? Maybe we can update the doc while we're at it.

@cmaddox5
Copy link
Contributor

cmaddox5 commented May 2, 2024

I notice the module doc refers to #ctd-pio-livepa

I think this webhook was overlooked when that channel was archived. The webhook still attempts to send messages there. I also don't have access to the channel that replaced it, and honestly these messages would probably be more appropriate in a channel like #screens-team-pios. Luckily this is something we set up just in case OIOs forget to close a takeover alert, but that situation has never happened. I can make a task to change both the Slack app and the code so it sends messages to a different TBD channel.

@digitalcora
Copy link
Contributor

but that situation has never happened.

Hopefully it would have failed noisily if it had ever tried to send a message to the archived channel.

Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

Looks good. One suggestion: would it be useful to conditionally add this job to application startup? The only environment that will ever need the job is prod so it might be worth preventing it from starting in other cases.

@digitalcora
Copy link
Contributor

I'd be hesitant about changing the process layout per-environment, because it increases the likelihood that we could accidentally break something and not notice until it surprises us in production.

@panentheos panentheos merged commit 376e09f into main May 3, 2024
4 checks passed
@panentheos panentheos deleted the bhw/slack-reminder branch May 3, 2024 13:12
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.

3 participants