-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Finalise environments targets and documentation #17095
Finalise environments targets and documentation #17095
Conversation
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
registries. | ||
This value may be any image identifier that the local Docker installation can accept. | ||
This includes image names with or without tags (e.g. `centos6` or `centos6:latest`), or | ||
image names with an immutable digest (e.g. `centos@sha256:<some_sha256_value>`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't specify an actual sha256 value here, as I think these docs are probably going to be subject to bitrot (basically preserving bugs or vulnerabilities that are out of our control within our docs). I imagine people familiar with Docker will be able to figure out what a good value here would be.
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
As mentioned on the ticket, I think that renaming the _environment=
field to environment=
now would also make sense, as long as the help string continues to reference that this is a preview.
Only one `local_environment` may be defined in `[environments-preview].names` per platform, and | ||
when `{LOCAL_ENVIRONMENT_MATCHER}` is specified as the environment, the | ||
`local_environment` that matches the current platform (if defined) will be selected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the environment
targets should include a doc URL for #17096 in their help: but if you're planning on landing this today, we can do that in a followup.
|
||
TODO: expectations about what are valid IDs, e.g. if they must come from DockerHub vs other | ||
registries. | ||
This value may be any image identifier that the local Docker installation can accept. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should possibly remind users to use a SHA for reproducibility?
Configuration of a Docker environment used for building your code, including the Docker | ||
image, along with environment-sensitive options, which include environment variables and | ||
search paths, used by Pants to execute processes in this Docker environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence has too many commas in it I think: not sure how to fix that. Break it up until multiple sentences, or make part of it parenthetical maybe?
Configuration of a remote execution environment used for building your code, including the | ||
environment variables and search paths used by Pants. | ||
environment-sensitive options, which include environment variables and | ||
search paths, used by Pants to execute processes in this remote environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto too many commas.
Adds documentation requested in #17095, and also renames the `_environment` field to `environment`.
This prepares the runtime environments support for preview release as part of 2.15:
_local
,_docker
, and_remote
}_environment
are now marked publicenvironments-preview
subsystem and_environment
field name remains as indicating preview status.Closes #7735