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

Fix resetting metadata on many repositories at once via the shed API #14906

Merged
merged 2 commits into from May 18, 2023

Conversation

jmchilton
Copy link
Member

The web controller method worked but the API method would collapse the revisions with metadata down to a single one. I suspected this when I opened #14890 but now I've brought in some test infrastructure from a downstream working branch to verify the bug and verify the fix. I've placed the tests and the fix in separate commits so the broken can be easily verified and so that we can backport the fix without tests if we'd like.

More About the Testing Code

One of my medium terms projects is to eliminate the complexity in the tool shed that allows uploading individual files and such to a repository. Planemo only uploads full archives and I would like the API for the tool shed to be this. This is how... all package managers work and since the introduction of Planemo we've been working to get away from the tool shed taking on other roles (i.e. source control management). This should simplify the tool shed and simplify what an alternative backend would have to support. In that direction, I would like to be able to quickly define like test repository lineages that can be uploaded for testing (both functional and unit tests). The old tool shed test code had some efforts in this direction but not as systematic and using tar files (which are difficult to work with and not ideal for sticking in git). All of that is to say that the column maker tools here are not just for these test cases but are about migrating the existing tests in better more manageable directions.

More About the Bug

rmm.set_repository resets resetting_all_metadata_on_repository and without that set the bug is expressed. This is troubling state management from a design perspective but I'm not going to fix it in a bug fix and is sort of endemic to these very stateful tool shed utilities (on the client and server side). The bug isn't exhibited for the web controller version of this functionality used by the UI because:

                    repository = repository_util.get_repository_in_tool_shed(self.app, repository_id)
                    self.set_repository(repository)
                    self.resetting_all_metadata_on_repository = True
                    self.reset_all_metadata_on_repository_in_tool_shed()

The consumer of the functionality re-resets the value back to the original after setting the repository.

How to test the changes?

(Select all options that apply)

./run_tests.sh --toolshed lib/tool_shed/test/functional/test_shed_repositories.py:TestShedRepositoriesApi.test_reset_all

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

log.debug(f"Resetting metadata on repository {repository.name}")
try:
rmm.set_repository(repository)
rmm = repository_metadata_manager.RepositoryMetadataManager(
Copy link
Member

Choose a reason for hiding this comment

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

I've looked at it couple time now, and I haven't figure out why exactly this fixes the bug. Is it because there's still something that set_repository didn't correctly reset to the right repository ? Given all the things the init method does this is definitely better, just curious at this point.

Copy link
Member Author

@jmchilton jmchilton Nov 2, 2022

Choose a reason for hiding this comment

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

😆 It isn't obvious at all - it took hours of debugging. set_repository resets resetting_all_metadata_on_repository to False, which has... implications for how working directories are used in generate_metadata_for_changeset_revision (in metadata_generator.py). When it resets, the actual directory of the repository is used and only the most recent contents of the repository are used as the working directory to take metadata from. So the tool shed is like "heres is all my tools and versions" over and over for each revision but the contents used to generate that are only the latest revision... and so only one interesting metadata repository is discovered.

@jmchilton jmchilton force-pushed the reset_metadata_testing branch 2 times, most recently from 2ed15db to db18d69 Compare November 3, 2022 17:02
@mvdbeek
Copy link
Member

mvdbeek commented Nov 4, 2022

Unsolicited advice: I often log in and sniff around by hand, https://github.com/mxschmitt/action-tmate#use-registered-public-ssh-keys is super useful for that.

@jmchilton jmchilton force-pushed the reset_metadata_testing branch 3 times, most recently from 15b2067 to 891704a Compare November 6, 2022 20:27
@jmchilton jmchilton force-pushed the reset_metadata_testing branch 8 times, most recently from f8105c5 to cd9ec5f Compare November 28, 2022 15:24
@jmchilton jmchilton force-pushed the reset_metadata_testing branch 6 times, most recently from d3ad2d2 to d8d7323 Compare November 29, 2022 18:28
@jmchilton jmchilton force-pushed the reset_metadata_testing branch 2 times, most recently from 26c5f29 to 8a8a2a5 Compare December 9, 2022 20:09
@jmchilton jmchilton force-pushed the reset_metadata_testing branch 2 times, most recently from 07557e4 to 3730929 Compare December 12, 2022 00:38
@jmchilton jmchilton marked this pull request as ready for review May 18, 2023 13:55
@jmchilton jmchilton merged commit fef73a4 into galaxyproject:dev May 18, 2023
38 of 39 checks passed
@github-actions github-actions bot added this to the 23.1 milestone May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants