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

[24.1] Revert some requests import changes #18199

Merged
merged 1 commit into from
May 24, 2024

Conversation

nsoranzo
Copy link
Member

@nsoranzo nsoranzo commented May 22, 2024

introduced in #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

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.

@@ -9,6 +9,8 @@
urlparse,
)

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.

I think we should add the header here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's only used to connect to Galaxy's /api/ga4gh/drs/v1/* and /api/drs_download/{object_id} URLs.

@@ -7,9 +7,9 @@
from unittest import SkipTest

import pytest
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.

Same, I think it's good to add the header here

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only used to make a PUT requests to Galaxy's /api/histories/{history_id}/contents/{item_id} .

@nsoranzo nsoranzo force-pushed the followup_on_18003 branch from 3188d25 to 0ec261b Compare May 22, 2024 08:44
@nsoranzo

This comment was marked as resolved.

@nsoranzo
Copy link
Member Author

Selenium test failure unrelated. Maintenance bot failure will be fixed by this PR once merged.

@mvdbeek
Copy link
Member

mvdbeek commented May 23, 2024

This should probably go to 24.1

@nsoranzo
Copy link
Member Author

This should probably go to 24.1

OK, then I'll split this PR in 2 since the GitHub actions changes apply only to dev. Unless we want to backport #18197 to 24.1?

@mvdbeek
Copy link
Member

mvdbeek commented May 23, 2024

We can also backport just what's needed.

@nsoranzo
Copy link
Member Author

Basically all of #18197 is needed to avoid issues with the end of Node 16 support: "On June 30th 2024, we will change the default from Node16 to Node20. To opt out of this and continue using Node16 while it is still available in the runner, you can choose to set ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION=true"

@mvdbeek
Copy link
Member

mvdbeek commented May 23, 2024

Then let's even target 24.0, since we'll have to support this until next year.

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 nsoranzo force-pushed the followup_on_18003 branch from cf84b85 to 8cdca0f Compare May 23, 2024 14:58
@nsoranzo nsoranzo changed the base branch from dev to release_24.1 May 23, 2024 14:59
@nsoranzo nsoranzo changed the title Revert some requests import changes [24.1] Revert some requests import changes May 23, 2024
@nsoranzo
Copy link
Member Author

OK, thanks for the feedback! I've removed the GH actions-related commits (will open a new PR against 24.0) and targeted this against 24.1 .

@nsoranzo
Copy link
Member Author

Failing integration test unrelated, ready to merge I think.

@mvdbeek mvdbeek merged commit 30829be into galaxyproject:release_24.1 May 24, 2024
58 of 59 checks passed
@nsoranzo nsoranzo deleted the followup_on_18003 branch May 24, 2024 14:39
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