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

WIP: Add ability to install plugins #69

Closed
wants to merge 7 commits into from

Conversation

connortann
Copy link
Contributor

@connortann connortann commented Feb 9, 2022

Closes #53

Adds ability to install plugins with:

- uses: snok/install-poetry@v1
  with:
    version: 1.2.0a2
    plugins: poetry-plugin-a, poetry-plugin-b

Might be worth adding a check that the Poetry version is at least 1.2 before trying to install plugins? I also note that version is not released yet, so possibly the syntax may change in future.

@connortann connortann changed the title Add ability to install plugins WIP: Add ability to install plugins Feb 9, 2022
@connortann connortann changed the title WIP: Add ability to install plugins Add ability to install plugins Feb 9, 2022
@sondrelg
Copy link
Member

sondrelg commented Feb 9, 2022

Do you think this is something we could add a test for, to verify that plugins are really installed @connortann? 🙂

@connortann
Copy link
Contributor Author

Oh nice, I didn't realise you had a test suite in GitHub workflows, I didn't think to look there. Certainly, I'll have a go! I'll need to think about to verify which plugins are installed; will do some experimentation.

action.yml Outdated Show resolved Hide resolved
@miigotu
Copy link
Collaborator

miigotu commented Feb 10, 2022

That release is yanked, it causes issues with other packages that require poetry.

@sondrelg
Copy link
Member

That release is yanked, it causes issues with other packages that require poetry.

The Poetry 1.2.0a2 release was yanked? I guess that won't impact the feature, but maybe we should use 1.2.0 for the docs example, even though it's not released.

Cheers for adding the test - that looks good to me @connortann 🙂 Would you like to add docs here as well? If you prefer I can add docs myself in a separate PR, just let me know 👍

@connortann
Copy link
Contributor Author

connortann commented Feb 10, 2022

Sure, I'll suggest adding something to the README. I also want to double-check the shell script I've added, so will set this PR as "WIP" for now.

Also, I noticed that the ShellCheck CI linter doesn't check the shell script within action.yml, so it would be nice to enable that if possible. Would it make sense for the bulk of the script within the action to call a main.sh ?

EDIT: another option could be to use a CI job to temporarily extract the shell scripts to a .sh file, before running ShellCheck

@connortann connortann changed the title Add ability to install plugins WIP: Add ability to install plugins Feb 10, 2022
@sonarcloud
Copy link

sonarcloud bot commented Feb 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sondrelg
Copy link
Member

I'd love to do this, but as far as I know (I hope I'm wrong), there's no way to bundle a main.sh script with the action. So if you called a script from action.yml that would break when run on an external repo. Dockerfile-based actions don't have this issue, since there you can just copy the relevant files into the container when building it.

Also wish there was something like this just with shellcheck and action.yml files 😛 If you can work out a way of getting this to work, I'd be very happy to accept it. I usually throw the code into a shell script, lint it, then copy it back.

@connortann
Copy link
Contributor Author

connortann commented Feb 10, 2022

I've attempted the migration to a main.sh on a separate PR #70 . I think it would be good to merge that one first; then I can rebase this PR accordingly.

EDIT: rebased this PR on top of main and fixed the conflicts. Nice having the ShellCheck linter working, it helped me avoid a few bash style issues.

action.yml Outdated Show resolved Hide resolved
plugins: poetry-version-plugin, poetry-export-plugin
- run: |
source .github/scripts/assert.sh
poetry plugin show
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The command poetry plugin show seems to be failing on CI, with what looks to be possibly an issue with Poetry itself? Any help debugging would be appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly related issue: python-poetry/poetry#4290

Copy link
Member

Choose a reason for hiding this comment

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

Agree this seems related. Do you think we'll need to wait for the next release candidate version to proceed with this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially, although that would be a shame. As an alternative, do you think it would be possible to "pip install" the plugins?

I think if Poetry is "pip-installed", then the plugins can also be pip-installed. Not sure though how this works when Poetry is installed with install-poetry.py ; perhaps by activating the venv and then using pip?

This seems a little non-standard; but, all the more reason for it to be implemented in this action so others don't have to deal with the same complexities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, we could comment out that part of the test for now, and implement the plugins feature, with a note in the README that the plugins system is still pre-release and subject to errors. I think our code is ready to go, for example I would use it in my project to install the dynamic-versioning plugin.

Copy link
Member

Choose a reason for hiding this comment

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

I've never really done a proper deep dive in the installation script, but on first glance it seems to just create a venv in an os-specific path (here-ish), so we certainly would have a pip-executable we could use to install things into that venv with.

Are Poetry plugins just added to the poetry venv anyway? In that case this seems like it would work 🤔

@sondrelg
Copy link
Member

Closing this for now @connortann, but feel free to reopen. Think most of the blockers are resolved now.

@sondrelg sondrelg closed this Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to install poetry plugins
4 participants