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

feat: obviate ghostlinkd #7336

Merged
merged 16 commits into from Apr 19, 2024
Merged

feat: obviate ghostlinkd #7336

merged 16 commits into from Apr 19, 2024

Conversation

rjsparks
Copy link
Member

No description provided.

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.84%. Comparing base (187c2c5) to head (6de2457).
Report is 101 commits behind head on main.

Files Patch % Lines
ietf/doc/views_charter.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7336      +/-   ##
==========================================
- Coverage   88.98%   88.84%   -0.15%     
==========================================
  Files         291      291              
  Lines       40717    40908     +191     
==========================================
+ Hits        36233    36344     +111     
- Misses       4484     4564      +80     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

Reviewed what's here. Agree that you've caught all the places in your comments in the first commit. A couple minor questions that might just reflect the wip-ness of this PR.

Also, I think you can remove your TODO in views_charter.py now?

dev/deploy-to-container/settings_local.py Show resolved Hide resolved
docker/configs/settings_local.py Show resolved Hide resolved
@rjsparks rjsparks marked this pull request as ready for review April 18, 2024 22:16
@rjsparks
Copy link
Member Author

rjsparks commented Apr 18, 2024

fwiw, for each unit, I left “bury the errors” vs “crash if there are errors” strategies as they are. Both of them bug me immensely. But to do a better thing in all these places (where the errors are really bad, and crashing is really bad) will require a bigger refactor.

@rjsparks
Copy link
Member Author

Also (for future us looking back at why we did this likely-to-look-foolish thing) - this moves a baked-in assumption into the datatracker from ghostlinkd (that the linked things are on the same filesystem and hardlinks will work).

Hopefully in the near future, we will keep all the things touched here in a blobstore instead of the filesystem and the above assumption (and the things this PR adds) will be simplified/rationalized into other applications feeding from that blobstore.

@jennifer-richards
Copy link
Member

this moves a baked-in assumption into the datatracker from ghostlinkd (that the linked things are on the same filesystem and hardlinks will work).

Still reviewing, but maybe this is an indication it's worth wrapping the hardlink operation in a update_in_archives() method that does whatever magic is appropriate? (Better name recommended, and this is not a fully baked suggestion.)

Copy link
Member

@jennifer-richards jennifer-richards 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, just a leftover "todo" comment to remove that I see.

WRT my comment about wrapping the link() operation - I think we'll end up wanting to do that, but I don't think it's necessary now. The main advantage is that I think it'd make the tests more independent, which might be slightly easier to change now. (Instead of separately testing that each place makes the right hard links, it'd just be a test that the hypothetical method was called, then a single test to update when that changes to a new mirroring strategy).

I don't think it'll be that much easier to do this now vs later, though.

ietf/idindex/tasks.py Outdated Show resolved Hide resolved
ietf/idindex/tasks.py Outdated Show resolved Hide resolved
@jennifer-richards jennifer-richards changed the title wip: identify whats needed to obviate ghostlinkd feat: obviate ghostlinkd Apr 19, 2024
@yasar456uhyhluih

This comment was marked as spam.

ietf/settings.py Outdated Show resolved Hide resolved
@rjsparks rjsparks merged commit cedd58f into ietf-tools:main Apr 19, 2024
9 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants