Skip to content

Conversation

@huonw
Copy link
Contributor

@huonw huonw commented Oct 5, 2022

This expands on #128 to add another bootstrap tool subcommand: bootstrap-version. This is designed to just be a simple number that's bumped whenever there's a feature scripts might need to query for. The version number is also automatically checked against the (numeric) value of the PANTS_BOOTSTRAP_TOOLS key.

For instance, suppose the bootstrap tools already existed (at version 1), and then bootstrap-cache-key command was added, the version number would be bumped to 2, and then a consumer (such as pantsbuild/actions#6) could run something like:

PANTS_BOOTSTRAP_CACHE_KEY=$(PANTS_BOOTSTRAP_TOOLS=2 ./pants bootstrap-cache-key)

Notes:

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I like it!

@benjyw
Copy link
Contributor

benjyw commented Oct 6, 2022

I think it's fine - window is small.

  • The checking could be packaged into the bootstrap script itself: the bootstrap script check that SCRIPT_VERSION >= PANTS_BOOTSTRAP_TOOLS, i.e. the example scenario above could be collapsed to just: PANTS_BOOTSTRAP_CACHE_KEY=$(PANTS_BOOTSTRAP_TOOLS=2 ./pants bootstrap-cache-key). Thoughts?

That could be neat! So the script runs if it's later than PANTS_BOOTSTRAP_TOOLS and fails otherwise?

@huonw
Copy link
Contributor Author

huonw commented Oct 6, 2022

That could be neat! So the script runs if it's later than PANTS_BOOTSTRAP_TOOLS and fails otherwise?

Yeah, exactly. I've implemented something along those lines.

@kaos kaos mentioned this pull request Oct 6, 2022
Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

LGTM mod the best-practice of bracing vars.

Co-authored-by: Benjy Weinberger <[email protected]>
@huonw
Copy link
Contributor Author

huonw commented Oct 7, 2022

Oops, thanks for that.

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.

3 participants