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

Add galaxy to user agent #18003

Merged
merged 10 commits into from
May 20, 2024
Merged

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Apr 16, 2024

Builds on #17578 but avoids global manipulation of requests. Should eventually also fix #17995

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 mvdbeek marked this pull request as ready for review May 20, 2024 15:54
@mvdbeek mvdbeek added this to the 24.1 milestone May 20, 2024
@mvdbeek mvdbeek requested a review from a team May 20, 2024 15:55
@jmchilton jmchilton merged commit 2f43c32 into galaxyproject:dev May 20, 2024
56 checks passed
@jmchilton
Copy link
Member

Very cool typing stuff in there! I'll have to revisit that and dig in more - looks great.

@nsoranzo nsoranzo deleted the add_galaxy_to_user_agent branch May 20, 2024 16:36
Copy link
Member

@nsoranzo nsoranzo left a comment

Choose a reason for hiding this comment

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

Sorry for belated review, if you agree these comments make sense, I can open a PR to fix them.

@@ -26,6 +26,7 @@
stream_to_file,
stream_url_to_file,
)
from galaxy.util import user_agent # noqa: F401
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed any more as well, I think?

@@ -133,6 +133,7 @@
build_tours_registry,
ToursRegistry,
)
from galaxy.util import user_agent # noqa: F401
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed any more, right?

@@ -4,7 +4,7 @@
import os
import urllib.parse

import requests
from galaxy.util import requests
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved after the gitlab import try/except.

import yaml

from galaxy.util import requests
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make much sense for this and the other scripts in scripts/api/ as they are meant to be example for how to connect to a Galaxy instance (not from a Galaxy server).

@@ -1,4 +1,4 @@
import requests
from galaxy.util import requests
Copy link
Member

Choose a reason for hiding this comment

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

This one was also not needed, I think.

@@ -68,6 +67,8 @@
Self,
)

from galaxy.util import requests
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved after the try/except imports (and can be rewritten as from . import requests).

@nsoranzo nsoranzo mentioned this pull request May 20, 2024
4 tasks
@galaxyproject galaxyproject deleted a comment from github-actions bot May 20, 2024
@mvdbeek
Copy link
Member Author

mvdbeek commented May 21, 2024

Yes, that'd be great, thank you @nsoranzo!

nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request May 22, 2024
introduced in galaxyproject#18003 .
In particular, when using requests in the tests to connect to Galaxy
or the ToolShed it's not needed to use the modified headers.

Also:
- Move some imports after conditional imports.
- Fix type annotations in `lib/tool_shed/test/base/populators.py`
- Fix typo bug in `test/unit/util/test_get_url.py`
nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request May 22, 2024
introduced in galaxyproject#18003 .
In particular, when using requests in the tests to connect to Galaxy
or the ToolShed it's not needed to use the modified headers.

Also:
- Move some imports after conditional imports.
- Fix type annotations in `lib/tool_shed/test/base/populators.py`
- Fix typo bug in `test/unit/util/test_get_url.py`
nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request May 23, 2024
introduced in galaxyproject#18003 .
In particular, when using requests in the tests to connect to Galaxy
or the ToolShed it's not needed to use the modified headers.

Also:
- Move some imports after conditional imports.
- Fix type annotations in `lib/tool_shed/test/base/populators.py`
- Fix typo bug in `test/unit/util/test_get_url.py`
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.

Specific URL fails to be downloaded from different galaxy instances
4 participants