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

Update the init-pants action to work with scie-pants. #16

Merged
merged 7 commits into from
Feb 8, 2023

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Feb 2, 2023

No description provided.

@benjyw
Copy link
Contributor Author

benjyw commented Feb 2, 2023

Note that to keep it simple, this new version doesn't support the old ./pants-based initialization.

If we want to avoid people haphazardly upgrading the action without first having switched to scie-pants, we can rename it, or give it a new version scheme.

@benjyw benjyw requested review from stuhood and jsirois February 2, 2023 17:14
@benjyw
Copy link
Contributor Author

benjyw commented Feb 2, 2023

Am manually testing this, but reviews welcome.

init-pants/action.yaml Show resolved Hide resolved
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I haven't used scie-pants yet. These changes look sane, I just have a few questions.

Does adding a description to inputs work now? There are some places that take a description, and others that don't. I believe I ran into issues with reusable workflows not allowing it, so I'm guessing that actions metadata allows it. Have you tested using this anywhere? Maybe in a PR in the example-python repo?

init-pants/action.yaml Outdated Show resolved Hide resolved
init-pants/action.yaml Show resolved Hide resolved
init-pants/action.yaml Show resolved Hide resolved
init-pants/action.yaml Outdated Show resolved Hide resolved
@benjyw
Copy link
Contributor Author

benjyw commented Feb 2, 2023

I'm testing this out, but I believe descriptions work.

@stuhood
Copy link
Member

stuhood commented Feb 3, 2023

Shouldn't the interpreter version be tied to the Pants version, rather than to scie-pants version? Or does that not matter in this case because scie-pants is doing the parsing and can emit that for you?

@jsirois
Copy link
Contributor

jsirois commented Feb 3, 2023

The latter:
https://github.com/pantsbuild/scie-pants/blob/7fc1471d6d0d66615fb0217459d5942cd161a799/tools/src/scie_pants/configure_pants.py#L120

Since scie-pants installs pantsbuild.pants in a venv it has to know the appropriate version of the hermetic PBS to use to do that install. When Pants switches distribution model to scie / PEX, if it chooses the (loose) scie route, Pants can completely control the Python it uses without letting scie-pants know. At that point this action will need to be updated to deal with that new state of the world.

- Use an in-repo get-pants.sh if available
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Thanks for adding the interpreter step back. I see one typo, and then LGTM

init-pants/action.yaml Outdated Show resolved Hide resolved
@benjyw benjyw merged commit ca4e0c4 into pantsbuild:main Feb 8, 2023
@benjyw benjyw deleted the scie_pants_action branch February 8, 2023 17:37
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.

5 participants