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

Switch to Consuming New Events API #50

Merged

Conversation

ThorntonMatthewD
Copy link
Collaborator

@ThorntonMatthewD ThorntonMatthewD commented Dec 6, 2023

Summary

Sets the Slack bot to pull events data from the new Events API that is/will be hosted at https://stage.hackgreenville.com/api/v0/events.

This updated implementation is brought about by hackgvl/hackgreenville-com#186 and the incredible work of @bogdankharchenko.

Please do not merge this PR until the changes are live at the aforementioned address.

@allella
Copy link
Member

allella commented Dec 6, 2023

Is / should this be an environment / configuration value instead of being hard coded?

@ThorntonMatthewD
Copy link
Collaborator Author

Is / should this be an environment / configuration value instead of being hard coded?

It's not, but it definitely can be!

@ThorntonMatthewD ThorntonMatthewD force-pushed the switch-to-new-events-api-staging branch from 82607de to 01bb64e Compare December 7, 2023 19:01
@ThorntonMatthewD ThorntonMatthewD marked this pull request as ready for review December 11, 2023 00:47
@ThorntonMatthewD
Copy link
Collaborator Author

I've posted a couple of minor issues I've found with the Slack Bot + new Events API on Slack here.

I'll post them here also:

  • https://stage.hackgreenville.com/api/v0/events?start_date=2023-12-03&end_date=2023-12-09 doesn't show the items for 12-09-2023, so the end date isn't inclusive (end_date=2023-12-09 is up to and including Dec 9th, 2023 00:00:00 which misses out on the rest of the day). It might be something we'd want to tweak, but since a consumer can just bump the day up one in their query I'm considering this a super low severity item.
  • The <p> tags that encapsulate event descriptions aren't trimmed like they appear to have been in the prior implementation, and so they show up in the Slack bot output.
    • There is a PR open for the Slack bot to trim HTML tags from the middle of descriptions: Removing html elements from events description. #19. It has kind of stalled out as the original contributor hasn't been responsive, so I'm wondering how we should proceed. It should fix this issue.
      • Should this sanitization be done server side or should every client of the API be responsible for cleaning up the descriptions as they see fit?
    • Also a very low severity

The date range params aren't currently used by the Slack bot so really the only issue is the extra HTML tags popping up in posts. Since work has already been started in #19 to fix HTML tags in descriptions in whole I am wondering what the next steps are to get that work ready for launch. 🚀

Overall I feel good about the prospect of this PR being merged, but I wanted to make sure I'm being transparent with potential concerns so that whomever reviews this piece can make an informed decision.

@ThorntonMatthewD ThorntonMatthewD force-pushed the switch-to-new-events-api-staging branch from c044bfb to 4734218 Compare December 11, 2023 13:22
@allella
Copy link
Member

allella commented Dec 11, 2023

@ThorntonMatthewD we don't currently have a .env or .envrc on the live system.

Should one of these be setup in advance of the next merge, or are these variables somehow securely sourced from inside the container using a key store?

@ThorntonMatthewD
Copy link
Collaborator Author

@allella How is the Slack token and signing secret currently stored? I can make sure the Events API URL is set up to pull from the same source for consistency.

@allella
Copy link
Member

allella commented Dec 11, 2023

The environment are in docker-compose.yml

services:
    slack-events-bots:
        environment:

@ThorntonMatthewD
Copy link
Collaborator Author

ThorntonMatthewD commented Dec 11, 2023

Okay, perfect! If you include this line in your local copy then that should take care of most of it. I'm not sure if watchtower will pick up on a change made in that manner, so a manual pull of the new image and a rebooting of the container may be in order (a docker-compose down and then docker compose-up -d).

It'll fall back to using the existing URL if for whatever reason the env var isn't able to be obtained.

@allella
Copy link
Member

allella commented Dec 11, 2023

Alright, the new - EVENTS_API_URL=https://stage.hackgreenville.com/api/v0/events has been added to the production.

Should we wait until this is merged to do the pull / down / up commands?

@ThorntonMatthewD
Copy link
Collaborator Author

Perfect! Thank you!

Yes, once this PR is merged and the workflow completes with uploading the new Docker image then it will be all set to cycle the container.

Copy link
Member

@oliviasculley oliviasculley 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!

@ThorntonMatthewD ThorntonMatthewD force-pushed the switch-to-new-events-api-staging branch from 4734218 to bfa1962 Compare December 12, 2023 19:12
@ThorntonMatthewD ThorntonMatthewD merged commit caf1f90 into hackgvl:dev Dec 12, 2023
1 check passed
@ThorntonMatthewD ThorntonMatthewD deleted the switch-to-new-events-api-staging branch December 12, 2023 19:14
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