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

[UsePythonV0] Download python from registry #15820

Merged
merged 36 commits into from
Mar 5, 2022

Conversation

DaniilShmelev
Copy link
Contributor

@DaniilShmelev DaniilShmelev commented Jan 25, 2022

Task name: UsePythonV0

Description: Currently, this task is only able to find local versions of python.
This PR adds a feature to download python from registry if it was not found locally.
This functionality is limited on self-hosted agents because:

  1. Proxy is not supported.
  2. The amount of supported distributions is limited.
  3. Self-hosted agents lack some permissions to install python versions.

Documentation changes required: Yes

Added unit tests: Yes

Attached related issue: #13319

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

@DaniilShmelev DaniilShmelev requested a review from a team January 25, 2022 08:23
@DaniilShmelev DaniilShmelev self-assigned this Jan 25, 2022
@DaniilShmelev
Copy link
Contributor Author

Works well on MS-hosted agents with the default images.
Does not work for self-hosted agents on other linux systems (debian, rhel) since the packages in the registry are meant to be used on ubuntu.
Also has issues with permissions on self-hosted agents.

Test run results

Copy link
Contributor

@EzzhevNikita EzzhevNikita left a comment

Choose a reason for hiding this comment

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

LGTM!
Performed manual testing of this task for installation of unstable versions of Python on all three platforms, everything working as expected.

@EzzhevNikita EzzhevNikita requested a review from a team February 2, 2022 14:02
@DaniilShmelev
Copy link
Contributor Author

@DaniilShmelev I've tested with the following Python version - '3.11.0', but it gives me 'Failed to download Python from the registry. Error: Error: Could not find Python matching spec 3.11.0 in the python-versions registry' - although it presents in the manifest. Should it consider partial package name match (and patterns like 3.11.x also)?

I don't see python '3.11.0' in the registry, only '3.11.0-alpha.1' and other alpha versions.

The task does support a partial package version spec like '3.10.x'. Unfortunately, semver cannot pick up versions with suffixes:

console.log(semver.satisfies('3.11.0-alpha.4', '3.11.x')); // false
console.log(semver.satisfies('3.11.0', '3.11.x')); // true

So, in this case, the customer needs to specify the version explicitly: '3.11.0-alpha.4'.
This is consistent with the setup-python action behavior: code.

Copy link
Contributor

@alexander-smolyakov alexander-smolyakov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Also, please take a look at the comments below.

Tasks/UsePythonVersionV0/osutil.ts Outdated Show resolved Hide resolved
Tasks/UsePythonVersionV0/task.loc.json Outdated Show resolved Hide resolved
@DaniilShmelev DaniilShmelev merged commit 983849e into master Mar 5, 2022
Copy link
Collaborator

@mmrazik mmrazik left a comment

Choose a reason for hiding this comment

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

approved

@maxim-lobanov
Copy link

@anatolybolshakov , @DaniilShmelev , how did you solve the rate-limit problem to download from GitHub releases?

Currently, task doesn't use any GH token and it performs unauthenticated requests. From GitHub docs: https://docs.github.com/en/rest/overview/resources-in-the-rest-api#requests-from-user-accounts

For unauthenticated requests, the rate limit allows for up to 60 requests per hour. Unauthenticated requests are associated with the originating IP address, and not the person making requests.

Azure VMs don't have unique IP addresses. There is a limited (and I think small) set of IP addresses that is shared between all machines. It means more than one machine (10, 100, 1000 machines) can have the same IP-address.
50 requests per multiple machines is very low limit and customers will easily hit it.

aasim pushed a commit that referenced this pull request Mar 8, 2022
* download python from registry if absent in cache

* add unit test

* fixes & debug messages

* fix manifest branch name

* fix debug message for python archive file name

* remove unneeded function

* fix unit tests failing on unix

* only check os version once

* add unit test for ubuntu version lookup

* adjust error messages

* add input to allow unstable versions

* fix ubuntu test

* add test for unstable version download

* add comments

* adjust comments

* select arch based on input

* extract interfaces into separate file

* make allowUnstable optional

* add a separate debug message for downloading

* explain what the registry is more clearly

* bump task version due to a new sprint

* fix allowUnstable default value

* format osutil to unify code style
aasim added a commit that referenced this pull request Mar 8, 2022
* Fixed casing for packaging api data

* Updated task version

* [Maven tasks] Bump the codecoverage-tools package at the dependencies (#15928)

* Update cc-tools package for MavenV2 task

* Update cc-tools package for MaveV3 task

* Disabled percent encoding (#15931)

* Update npmauth to handle retries cleanly (#15914)

* Update npmauth.ts

* Update task.json

* Update task.loc.json

* Update npmauth.ts

* RED

Co-authored-by: Your Name <[email protected]>

* Updated Helm download Url (#15821)

* updated helm download url

* bumped task vesrions required for this change

* bumped task versions

* [FtpUploadV2] Resolve dependabot alert about shelljs (#15881)

* fix dependencies

* fix build

build error are caused by tasklib 3.x return types change from "<type>" to "<type> | undefined"
functionally, these method calls should be identical

* bump task version

* Add simple unit tests to check getting input parameters and start of task

* Delete debug

* Fix EOL

* Updated task version to 201

* Added test for successful execution

Co-authored-by: Tatyana Kostromskaya (Akvelon) <[email protected]>
Co-authored-by: Tatyana Kostromskaya <[email protected]>
Co-authored-by: Denis Tikhomirov <[email protected]>
Co-authored-by: Denis Tikhomirov <[email protected]>
Co-authored-by: Anatoly Bolshakov <[email protected]>

* Fix README.md section on installing Invoke-SqlCommand on an agent (#15693)

* Add another way of installing Invoke-SqlCommand.
* Fix dead link.

Co-authored-by: Kim Jämiä <[email protected]>
Co-authored-by: Philipson Joseph V <[email protected]>

* updating  the azure-pipelines-tasks-azure-arm-rest-v2 to 2.198.2  (#15935)

* version update of common module

* version update

Co-authored-by: rajnish-byte <[email protected]>

* Fix shelljs vulnerability for tasks (#15972)

* bump shelljs for ArchiveFilesV2

* bump shelljs for BashV3

* bump shelljs for CMakeV1

* bump shelljs for CocoaPodsV0

* bump affected tasks versions

* 15646 - Machine Names required", but should not be required, and machine names being prepended to destination folder #15646 (#15870)

* Making Machines,AdminLogin,Password as Mandatory

* Making Machines,Admin Login and Password required fields

* fix no wait result bug (#15957)

* fix no wait result bug

* bump version

Co-authored-by: Anatoly Bolshakov <[email protected]>
Co-authored-by: Nikita Ezzhev <[email protected]>

* [UsePythonV0] Download python from registry (#15820)

* download python from registry if absent in cache

* add unit test

* fixes & debug messages

* fix manifest branch name

* fix debug message for python archive file name

* remove unneeded function

* fix unit tests failing on unix

* only check os version once

* add unit test for ubuntu version lookup

* adjust error messages

* add input to allow unstable versions

* fix ubuntu test

* add test for unstable version download

* add comments

* adjust comments

* select arch based on input

* extract interfaces into separate file

* make allowUnstable optional

* add a separate debug message for downloading

* explain what the registry is more clearly

* bump task version due to a new sprint

* fix allowUnstable default value

* format osutil to unify code style

* Updated version

Co-authored-by: Aasim Malladi <[email protected]>
Co-authored-by: Konstantin Tyukalov <[email protected]>
Co-authored-by: Anatoly Bolshakov <[email protected]>
Co-authored-by: Marcin Strzyz <[email protected]>
Co-authored-by: Your Name <[email protected]>
Co-authored-by: rajnish-byte <[email protected]>
Co-authored-by: Daniil Shmelev <[email protected]>
Co-authored-by: Tatyana Kostromskaya (Akvelon) <[email protected]>
Co-authored-by: Tatyana Kostromskaya <[email protected]>
Co-authored-by: Denis Tikhomirov <[email protected]>
Co-authored-by: Denis Tikhomirov <[email protected]>
Co-authored-by: Kim Jämiä <[email protected]>
Co-authored-by: Kim Jämiä <[email protected]>
Co-authored-by: Philipson Joseph V <[email protected]>
Co-authored-by: v-hinti <[email protected]>
Co-authored-by: v-nagarajku <[email protected]>
Co-authored-by: Ruoyu Wang <[email protected]>
Co-authored-by: Nikita Ezzhev <[email protected]>
DaniilShmelev added a commit that referenced this pull request Mar 10, 2022
@DaniilShmelev
Copy link
Contributor Author

DaniilShmelev commented Mar 10, 2022

@maxim-lobanov thanks, prepared a revert PR along with the fix proposal (1, 2).

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.

6 participants