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

ASDF_POETRY_INSTALL_URL: Optionally override installer URL #16

Merged
merged 2 commits into from
Nov 28, 2021

Conversation

tony
Copy link
Contributor

@tony tony commented Nov 27, 2021

Escape hatch for #14 issue (python 3.10 support for >=1.1.9;<1.2) while preserving the get-poetry.py default behavior. This only changes behavior when ASDF_POETRY_INSTALL_URL is passed in. Example:

If you haven't, uninstall: asdf uninstall poetry 1.1.8;

See also: asdf-python's environmental variable e.g. ASDF_PYTHON_PATCH_URL, asdf-nodejs's environmental variable e.g. NODEJS_CHECK_SIGNATURES

Default installer for <1.2 (get-poetry.py):

asdf install poetry 1.1.8

Uninstall again:

asdf uninstall poetry 1.1.8;

Notice the warning:

This installer is deprecated. Poetry versions installed using this
script will not be able to use 'self update' command to upgrade to
1.2.0a1 or later.

Force install-poetry.py:

ASDF_POETRY_INSTALL_URL=https://install.python-poetry.org asdf install poetry 1.1.8

@tony tony requested a review from a team as a code owner November 27, 2021 23:37
If you haven't, uninstall: asdf uninstall poetry 1.1.8;

Default installer for <1.2 (get-poetry.py):

asdf install poetry 1.1.8

Uninstall again:

asdf uninstall poetry 1.1.8;

Notice the warning:

> This installer is deprecated. Poetry versions installed using this
> script will not be able to use 'self update' command to upgrade to
> 1.2.0a1 or later.

Force install-poetry.py:

ASDF_POETRY_INSTALL_URL=https://install.python-poetry.org asdf install poetry 1.1.8
@tony
Copy link
Contributor Author

tony commented Nov 27, 2021

@crflynn @Kurt-von-Laven: Thoughts on this approach in favor of #14?

All existing behavior is the same. If ASDF_POETRY_INSTALL_URL is set the installer can be overridden.

@Kurt-von-Laven
Copy link
Contributor

I do like this better than a --classic flag. The way you have specified the environment variable is certainly very flexible, but users may prefer the convenience of a boolean environment variable to save keystrokes since the Poetry install URL is intended to be stable for the foreseeable future. I defer to someone with more asdf experience in any case since I have no idea whether there is an existing convention in the ecosystem for these sorts of scenarios.

@crflynn
Copy link
Member

crflynn commented Nov 28, 2021

This looks OK although I tend to agree with Kurt on preferring a boolean over a URL as I don't think there would be other options for the URL. I don't plan on using this anyway so in that regard I'm indifferent.

I'll just note that if newer versions of poetry revert to the original method of detecting python versions (and have better compatibility in general) I will re-evaluate and likely remove this feature, since I consider it a footgun as it allows for broken installations of poetry with respect to asdf functionality via #10.

@tony
Copy link
Contributor Author

tony commented Nov 28, 2021

@crflynn Have a preference on the variable name?

@crflynn
Copy link
Member

crflynn commented Nov 28, 2021

The current implementation is fine I suppose. Could you please document this under #usage in the readme?

@tony
Copy link
Contributor Author

tony commented Nov 28, 2021

@crflynn To clarify one thing:

The current implementation is fine I suppose.

By that, does that mean keep it as-is (where install URL is specified) or keep the variable name the same and switch it to a boolean?

Could you please document this under #usage in the readme?
Noted

@crflynn
Copy link
Member

crflynn commented Nov 28, 2021

Lets just keep it as-is.

@tony tony force-pushed the tn-asdf-poetry-install-url branch 3 times, most recently from 331f2f2 to e036b92 Compare November 28, 2021 20:58
@tony
Copy link
Contributor Author

tony commented Nov 28, 2021

@crflynn How is this? e036b92 (markdown, view)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@crflynn crflynn merged commit 29ca741 into asdf-community:master Nov 28, 2021
@tony tony deleted the tn-asdf-poetry-install-url branch November 28, 2021 21:22
@tony
Copy link
Contributor Author

tony commented Nov 28, 2021

@crflynn Thank you for the review! @Kurt-von-Laven thank you for checking into both this and #14 as well

@Kurt-von-Laven
Copy link
Contributor

Thanks for actually doing the work and coming up with a reasonable workaround to a sticky situation. I'm sure everyone will be looking forward to the next Poetry 1.2 release candidate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants