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

[23.2] Build param dict before creating entrypoint #17440

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Feb 8, 2024

Fixes #17438.

Thanks for the excellent debugging @cat-bro!

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]

License

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

@mvdbeek
Copy link
Member Author

mvdbeek commented Feb 8, 2024

@sveinugu does this still make sense to you ? I'm not sure why you moved the entry point creation

@github-actions github-actions bot added this to the 23.2 milestone Feb 8, 2024
@sveinugu
Copy link
Contributor

sveinugu commented Feb 9, 2024

Right. So it seems there is a circular dependency afoot!

Moving populate_interactivetools() was crucial to allow injection of the interactive tool entry point path to the IT container, which again is crucial to get path-based interactive tools to work in most cases. So one needs to configure the interactive tool entry points before the entry point paths are to be injected to the job as e.g. an environment variable, such as in the following example:

<entry_points>
    <entry_point name="Display name" label="mytool" requires_domain="False" requires_path_in_url="True">
        <port>80</port>
    </entry_point>
</entry_points>

<environment_variables>
    <environment_variable name="EP_PATH" inject="entry_point_path_for_label">mytool</environment_variable>
</environment_variables>

(from https://docs.galaxyproject.org/en/master/admin/special_topics/interactivetools.html).

The only thing this breaks is the ability to use templates for the name of the entry points of ITs, which only seems to be used to put the name of the infile into the name of the entrypoint ("Display name" above). This again I believe is only really used for the "Active Interactive Tools" table (the title of the new window/tab is something else). So one alternative fix is to just remove this templating from all the ITs that use it, and not allow templating of entry point names. A more complicated fix is to refactor somehow to support both dependencies

@sveinugu
Copy link
Contributor

sveinugu commented Feb 9, 2024

Just to be clear: merging this PR would break path-based interactive tools!

@mvdbeek mvdbeek force-pushed the fix_it_templating branch 2 times, most recently from 64ba558 to b765262 Compare February 10, 2024 11:42
@mvdbeek
Copy link
Member Author

mvdbeek commented Feb 10, 2024

OK, I've moved the entry point creation so that the param_dict is available, but before templating out environment variables and config files.

@sveinugu
Copy link
Contributor

LGTM. Thanks!

@mvdbeek mvdbeek force-pushed the fix_it_templating branch 2 times, most recently from eb16c67 to 5cc1249 Compare February 10, 2024 16:44
@mvdbeek
Copy link
Member Author

mvdbeek commented Feb 12, 2024

Test failures are unrelated

@mvdbeek mvdbeek requested a review from a team February 12, 2024 14:52
@mvdbeek mvdbeek requested a review from nsoranzo February 13, 2024 14:10
@nsoranzo nsoranzo merged commit f4229aa into galaxyproject:release_23.2 Feb 13, 2024
43 of 46 checks passed
@nsoranzo nsoranzo deleted the fix_it_templating branch February 13, 2024 16:41
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.

3 participants