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

Replace activity stream with S3 approach #471

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

chopkinsmade
Copy link
Collaborator

  • JIRA ticket referenced in title
  • Title is clear and concise
  • Description gives any relevant detail
  • Tests are up to date
  • Documentation is up to date

.localstack/Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@marcelkornblum marcelkornblum 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 submitting this; we'd like to have a chat about it for sure :)

Firstly, we are trying to understand what this work is about - it seems to replace the call to Staff SSO's ActivityStream API, but not do anything about the proxy DB that I thought was the whole point of the work? I might have the wrong end of the stick though.

Secondly, this PR is missing a lot of documentation; the repo needs to have a section explaining how this all works, down to the level of what files ought to be in written to the bucket, what's in them and how to process them.

Typing would make the code (especially in utils) much easier to understand and debug; we definitely want to make the utils code typed.

Most of the comments below are very picky around naming or structure to fit in with our approaches; the overall approach looks good.

I would also like to game out the high level approach together; there are a few downsides to implementing this that I want to chat about including error correction and what happens if we miss an ingest or get partial records, and how we might be able to move to a nearly-live system in the future if we wanted to.

Mostly though I think we just want to understand the intention of this code since it doesn't address what I thought we were trying to address with this work (i.e. the proxy DB is unaffected).

config/celery.py Outdated Show resolved Hide resolved
.localstack/Dockerfile Outdated Show resolved Hide resolved
config/settings/base.py Outdated Show resolved Hide resolved
activity_stream/utils.py Outdated Show resolved Hide resolved
activity_stream/utils.py Outdated Show resolved Hide resolved
activity_stream/utils.py Outdated Show resolved Hide resolved
activity_stream/utils.py Outdated Show resolved Hide resolved
activity_stream/utils.py Outdated Show resolved Hide resolved
@chopkinsmade chopkinsmade force-pushed the feature/switch-to-S3-for-activity-stream-users branch from db2dfc4 to 0d2da87 Compare November 8, 2024 11:31
@CamLamb
Copy link
Contributor

CamLamb commented Dec 16, 2024

I have tested this PR locally and I'm happy the code is functioning, there are still a couple of steps I'd like to do next:

  • Deploy to an environment and run some tests to ensure the code functions with a real S3 instance
  • Uncomment/re-write the tests added in this PR

@CamLamb CamLamb changed the title WIP: switch-to-S3-for-activity-stream-users Replace activity stream with S3 approach Jan 2, 2025
@CamLamb CamLamb marked this pull request as ready for review January 2, 2025 13:09
@CamLamb CamLamb requested a review from a team as a code owner January 2, 2025 13:09
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