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

Remove package from repodata.json #551

Merged
merged 6 commits into from
Jul 27, 2022
Merged

Conversation

brichet
Copy link
Collaborator

@brichet brichet commented Jul 25, 2022

This PR cleans the repodata.json files in all platforms which was containing the removed package.

Fix #475

@wolfv
Copy link
Member

wolfv commented Jul 25, 2022

nice, thanks for this PR!

I think we should add tests to make sure the two issues are properly fixed.

@brichet
Copy link
Collaborator Author

brichet commented Jul 26, 2022

Sure.
In fact I'm not sure there are already some tests on repodata.json content.

@brichet brichet added the bug Something isn't working label Jul 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Merging #551 (7930c17) into main (1d2feb9) will decrease coverage by 0.81%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
- Coverage   82.79%   81.97%   -0.82%     
==========================================
  Files          78       78              
  Lines        5964     5971       +7     
==========================================
- Hits         4938     4895      -43     
- Misses       1026     1076      +50     
Impacted Files Coverage Δ
quetz/main.py 86.16% <100.00%> (+0.05%) ⬆️
quetz/tasks/indexing.py 93.42% <100.00%> (+2.20%) ⬆️
quetz/pkgstores.py 42.45% <0.00%> (-11.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d2feb9...7930c17. Read the comment docs.

@wolfv
Copy link
Member

wolfv commented Jul 27, 2022

@brichet it's easiest if you install pre-commit and then do pre-commit run --all and it will fix everything for you with regards to linting.

@brichet
Copy link
Collaborator Author

brichet commented Jul 27, 2022

@brichet it's easiest if you install pre-commit and then do pre-commit run --all and it will fix everything for you with regards to linting.

Thanks

@brichet
Copy link
Collaborator Author

brichet commented Jul 27, 2022

As there are no test on repodata.json file content yet, don't you think we can do that in another PR.
It would include some tests on creation, modification and deletion.

@wolfv
Copy link
Member

wolfv commented Jul 27, 2022

We can, but I would like to put it very high on the priority list.

@@ -85,9 +95,12 @@ def test_delete_package_versions_with_package(

assert len(versions) == 0

files = pkgstore.list_files(public_channel.name)
for filename in package_filenames:
init_files.remove(filename)
Copy link
Member

Choose a reason for hiding this comment

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

this should not happen when the subdir is noarch because otherwise the client thinks that the channel does not exist.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure to understand.
This code removes the package filenames from the list of channel files (repodata.json, index.html...), to compare the files list before an after removing a package (only the package archive should have been removed).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I thought that you were deleting empty repodata.json files. I should've looked more carefully.

Copy link
Member

Choose a reason for hiding this comment

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

Clean repodata.json from empty platform when deleting a package

That was the commit message that confused me :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem, thanks for review

@brichet
Copy link
Collaborator Author

brichet commented Jul 27, 2022

We can, but I would like to put it very high on the priority list.

OK, I'm starting updating the tests anyway, it can be on that PR.