Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Onboard Sentry #54

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Onboard Sentry #54

merged 1 commit into from
Aug 27, 2024

Conversation

chadwhitacre
Copy link
Contributor

No description provided.

Copy link
Member

@vladh vladh left a comment

Choose a reason for hiding this comment

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

@chadwhitacre The JSON file is missing datetimeModified, which makes checking for new changes impossible — could we add it?

@chadwhitacre
Copy link
Contributor Author

Ah, I was expecting that we would add that on our side. Wdyt?

@vladh
Copy link
Member

vladh commented Aug 27, 2024

Hmm, so the problem we want to solve here is: detecting when a member has changed their JSON file.

Either way we have to: fetch from the JSON file URL periodically.

To detect changes, we can either:

  1. check if datetimeModified is newer, or
  2. check if the JSON is different from what we have locally.

The code currently does (1). If you'd like to do (2), I can change the code to do that, and we can remove datetimeModified.

I would personally do (1) and keep datetimeModified, because without it, while we can check that a member's JSON file is different, we can't know that it's newer. But, (2) is probably also Good Enough™.

@chadwhitacre
Copy link
Contributor Author

Or should we just drop datetimeModified entirely? Does it matter? How are we going to use it?

I think my thought in adding it originally is that we could use it to implement a more complex algorithm for fetching JSON files, something like: check datetimeModified and if it's recent don't refetch for a month or whatever. That now seems like overcomplication to me and probably we can just drop/ignore it entirely. Eh?

@chadwhitacre
Copy link
Contributor Author

Seeing how we're currently using it:

https://github.com/opensourcepledge/osspledge.com/blob/c6dc6b5e5682019838592006d2980ba6c3520b91/bin/update-member#L38-L42

Since this doesn't save us from the fetch, it seems we could probably just always save the file contents and let git do the work of picking up changes.

@vladh
Copy link
Member

vladh commented Aug 27, 2024

Cool, let's just assume changed=newer and drop datetimeModified entirely, I'll prepare a PR, then I'll merge this!

@chadwhitacre
Copy link
Contributor Author

#55 :)

@vladh
Copy link
Member

vladh commented Aug 27, 2024

That was speedy!

@vladh vladh force-pushed the cwlw/onboard-sentry branch 2 times, most recently from 4787841 to 6649ae6 Compare August 27, 2024 16:04
@vladh
Copy link
Member

vladh commented Aug 27, 2024

Tested and LGTM, merging.

@vladh vladh merged commit cd9bea0 into main Aug 27, 2024
2 checks passed
@vladh vladh deleted the cwlw/onboard-sentry branch August 27, 2024 16:04
@vladh
Copy link
Member

vladh commented Aug 27, 2024

We forgot to rename paymentsToProjects to payments, did that on main in 33a42fc.

@vladh
Copy link
Member

vladh commented Aug 27, 2024

Also should have removed domain, done in dcda264 — it was unused anyway.

@vladh
Copy link
Member

vladh commented Aug 27, 2024

I also made a few improvements to deploy.yml on main.

Deployment done, LGTM! https://osspledge.com/members/sentry/

@chadwhitacre
Copy link
Contributor Author

👍 🎉

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants