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

[22.01] Fix container resolution for type="singularity" requirements #15893

Conversation

bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Apr 4, 2023

otherwise container_description.identifier = cache_path (at the end of the function) will modify the original (i.e. the tools) container description which influences i.e. (may break) successive container resolutions.

Bug occured only for containers of type="singularity" .. for explicit docker container requirements a copy was already created by dictifying and undictifying it.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

Found this while working on #15614 .. tests are coming.

License

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

@github-actions github-actions bot added this to the 22.05 milestone Apr 4, 2023
@bernt-matthias bernt-matthias force-pushed the fix/cached_explicit_singularity_container_description branch 2 times, most recently from 9e9ee89 to e81fc67 Compare April 4, 2023 10:07
otherwise `container_description.identifier = cache_path` (at the end of the function)
will modify the original (i.e. the tools) container description which influences
i.e. (may break) successive container resolutions.
@mvdbeek mvdbeek merged commit f76d894 into galaxyproject:release_22.01 Apr 4, 2023
@github-actions
Copy link

github-actions bot commented Apr 4, 2023

This PR was merged without a "kind/" label, please correct.

@mvdbeek mvdbeek changed the title [22.01] cached_explicit_singularity: needs to copy container description [22.01] Fix container resolution for type="singularity" requirements Apr 4, 2023
@mvdbeek mvdbeek changed the title [22.01] Fix container resolution for type="singularity" requirements [22.01] Fix container resolution for type="singularity" requirements Apr 4, 2023
@mvdbeek
Copy link
Member

mvdbeek commented Apr 4, 2023

otherwise container_description.identifier = cache_path (at the end of the function) will modify the original (i.e. the tools) container description which influences i.e. (may break) successive container resolutions.

thanks, as a minor comment I would put this as a comment above where you create the copy

@bernt-matthias bernt-matthias deleted the fix/cached_explicit_singularity_container_description branch April 4, 2023 12:43
@bernt-matthias
Copy link
Contributor Author

thanks, as a minor comment I would put this as a comment above where you create the copy

Thanks for the feedback. I can do this. Should I open a new PR for this?

@mvdbeek
Copy link
Member

mvdbeek commented Apr 4, 2023

Can you merge the release branches forward ? I think that'd be a good idea, but should be enough to just have this in dev

@bernt-matthias
Copy link
Contributor Author

Can you merge the release branches forward ?

Never did this. Would this be just git merge release_22.01 while on dev?

@dannon
Copy link
Member

dannon commented Apr 4, 2023

@bernt-matthias One branch at a time; 22.01 -> 22.05, 22.05 -> 23.0, 23.0 -> dev.

@mvdbeek
Copy link
Member

mvdbeek commented Apr 4, 2023

No, merge into 22.05, push, then merge that into 23.0, push, then merge that into dev

@bernt-matthias bernt-matthias mentioned this pull request Apr 4, 2023
4 tasks
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

3 participants