Add validation for pants_version format #338
Conversation
|
Fix #337 |
There was a problem hiding this comment.
Thanks very much for taking the time to contribute!
As you noted on slack #156 is quite similar. I wonder if we could take more of a "post-hoc" explanation approach here, similar to that one. As in, 'trust' the version more, until we get to a point where a network request definitely fails, and then offer explanations of what might've gone wrong.
So, I think this means:
- for versions 2.17 and early, merging Improve error messaging on bad Pants version #156 and updating it (potentially doing things like: if there's only one dot, specifically suggest adding the
.patch) - for versions 2.18 and later, adjusting the following code to detect the missing patch too.
scie-pants/tools/src/scie_pants/install_pants.py
Lines 108 to 115 in 7691160
It would also be good to add tests, similar to #156.
(I'm a bit busy so I can't provide lots of info promptly, but please ask questions!)
| @@ -99,6 +99,11 @@ def determine_find_links( | |||
| def determine_tag_version( | |||
| ptex: Ptex, pants_version: str, find_links_dir: Path, github_api_bearer_token: str | None = None | |||
| ) -> ResolveInfo: | |||
| # `pants_version` should be in the format `major.minor.micro`, e.g., `2.17.0`. | |||
| # Raise an error if it does not follow this format. | |||
| if not pants_version.count(".") == 2: | |||
There was a problem hiding this comment.
FYI, a dev release has a format with more .s, so we'll need to refine this somewhat to support them. For instance: https://github.com/pantsbuild/pants/releases/tag/release_2.20.0.dev3
This is part of why I think we should wait until a network request fails, to be resilient to whatever version format the upstream releases choose to use.
There was a problem hiding this comment.
I just improved the error message and added a test. Please take a look! Thank you.
| @@ -105,8 +105,15 @@ def install_pants_from_pex( | |||
| try: | |||
| ptex.fetch_to_fp(pex_url, pants_pex.file) | |||
| except subprocess.CalledProcessError as e: | |||
| # if there's only one dot in version, specifically suggest adding the `.patch`) | |||
| suggestion = ( | |||
| "Pants version format not recognized. Please add `.<patch_version>` to the end of the version. For example: `2.18` -> `2.18.0`" | |||
There was a problem hiding this comment.
I'd suggest we make two minor adjustments:
- have this be a full sentence (i.e.
.on the end) - ensure that if there's no suggestion, we don't end up with 3 blank lines, by including the
\n\nhere
| "Pants version format not recognized. Please add `.<patch_version>` to the end of the version. For example: `2.18` -> `2.18.0`" | |
| "Pants version format not recognized. Please add `.<patch_version>` to the end of the version. For example: `2.18` -> `2.18.0`.\n\n" |
Then change f"{suggestion}\n\n" to be just f"{suggestion}" or maybe include {suggestion} at the end of the first line.
There was a problem hiding this comment.
Thank so much! I just pushed another commit. Please check!
Implemented a check to ensure the 'pants_version' follows the 'major.minor.micro' format. This update raises a clear error message if the version string does not meet the expected format, enhancing user guidance and preventing configuration errors.
Fix #337