-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
Add fallback_environment
for remote_environment
#16955
Changes from 3 commits
0bb483f
c8c6bc4
6ea325c
c104e79
02e008f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,12 +204,37 @@ class RemoteExtraPlatformPropertiesField(StringSequenceField): | |
) | ||
|
||
|
||
_NO_FALLBACK_ENVIRONMENT = "<none>" | ||
|
||
|
||
class RemoteFallbackEnvironmentField(StringField): | ||
alias = "fallback_environment" | ||
default = LOCAL_ENVIRONMENT_MATCHER | ||
value: str | ||
help = softwrap( | ||
f""" | ||
The environment to fallback to when remote execution is disabled via the global option | ||
`--remote-execution`. | ||
|
||
Must be an environment name from the option `[environments-preview].names`, the | ||
special string `{LOCAL_ENVIRONMENT_MATCHER}` to use the relevant local environment, or the | ||
string `{_NO_FALLBACK_ENVIRONMENT}` to error when remote execution is disabled. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that falling back by default to the local environment is error prone, so I think that this is potentially inverted. The chances of a local environment being compatible with the remote environment without extra consideration/attention paid to setting up all of the env vars correctly is pretty slim: and it seems better to fail fast in that case ("you probably haven't made this compatible yet"), rather than to fail slowly ("I fell back automatically for you, and now something is failing in an unexpected way mid build"). Having this be |
||
|
||
Tip: if you are using a Docker image with your remote execution environment (usually | ||
enabled by setting the field {RemoteExtraPlatformPropertiesField.alias}`), then it can be | ||
useful to fallback to an equivalent `docker_image` target so that you have a consistent | ||
execution environment. | ||
""" | ||
) | ||
|
||
|
||
class RemoteEnvironmentTarget(Target): | ||
alias = "_remote_environment" | ||
core_fields = ( | ||
*COMMON_TARGET_FIELDS, | ||
RemotePlatformField, | ||
RemoteExtraPlatformPropertiesField, | ||
RemoteFallbackEnvironmentField, | ||
) | ||
help = softwrap( | ||
""" | ||
|
@@ -252,6 +277,10 @@ class UnrecognizedEnvironmentError(Exception): | |
pass | ||
|
||
|
||
class RemoteExecutionDisabledError(Exception): | ||
pass | ||
|
||
|
||
class AllEnvironmentTargets(FrozenDict[str, Target]): | ||
"""A mapping of environment names to their corresponding environment target.""" | ||
|
||
|
@@ -382,7 +411,9 @@ async def determine_local_environment( | |
|
||
@rule | ||
async def resolve_environment_name( | ||
request: EnvironmentNameRequest, environments_subsystem: EnvironmentsSubsystem | ||
request: EnvironmentNameRequest, | ||
environments_subsystem: EnvironmentsSubsystem, | ||
global_options: GlobalOptions, | ||
) -> EnvironmentName: | ||
if request.raw_value == LOCAL_ENVIRONMENT_MATCHER: | ||
local_env_name = await Get(ChosenLocalEnvironmentName, {}) | ||
|
@@ -399,6 +430,38 @@ async def resolve_environment_name( | |
""" | ||
) | ||
) | ||
env_tgt = await Get(EnvironmentTarget, EnvironmentName(request.raw_value)) | ||
if env_tgt.val is None: | ||
raise AssertionError(f"EnvironmentTarget.val is None for the name `{request.raw_value}`") | ||
|
||
# If remote execution is disabled and it's a remote environment, try falling back. | ||
if not global_options.remote_execution and env_tgt.val.has_field( | ||
RemoteFallbackEnvironmentField | ||
): | ||
fallback_field = env_tgt.val[RemoteFallbackEnvironmentField] | ||
if fallback_field.value == _NO_FALLBACK_ENVIRONMENT: | ||
raise RemoteExecutionDisabledError( | ||
softwrap( | ||
f""" | ||
The global option `--remote-execution` is set to false, but the remote | ||
environment `{request.raw_value}` is used in {request.description_of_origin}. | ||
|
||
Either enable the option `--remote-execution`, or set the field | ||
`{fallback_field.alias}` for the target {env_tgt.val.address}, | ||
e.g. to the default `{LOCAL_ENVIRONMENT_MATCHER}`. | ||
""" | ||
) | ||
) | ||
return await Get( | ||
EnvironmentName, | ||
EnvironmentNameRequest( | ||
fallback_field.value, | ||
description_of_origin=( | ||
f"the `{fallback_field.alias}` field of the target {env_tgt.val.address}" | ||
), | ||
), | ||
) | ||
|
||
return EnvironmentName(request.raw_value) | ||
|
||
|
||
|
@@ -433,12 +496,16 @@ async def get_target_for_environment_name( | |
WrappedTargetRequest(address, description_of_origin=_description_of_origin), | ||
) | ||
tgt = wrapped_target.val | ||
if not tgt.has_field(CompatiblePlatformsField) and not tgt.has_field(DockerImageField): | ||
if ( | ||
not tgt.has_field(CompatiblePlatformsField) | ||
and not tgt.has_field(DockerImageField) | ||
and not tgt.has_field(RemotePlatformField) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. This blocked RE targets actually being usable |
||
): | ||
raise ValueError( | ||
softwrap( | ||
f""" | ||
Expected to use the address to a `_local_environment` or `_docker_environment` | ||
target in the option `[environments-preview].names`, but the name | ||
Expected to use the address to a `_local_environment`, `_docker_environment`, or | ||
`_remote_environment` target in the option `[environments-preview].names`, but the name | ||
`{env_name.val}` was set to the target {address.spec} with the target type | ||
`{tgt.alias}`. | ||
""" | ||
|
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.
With
None
as the default, this value is no longer necessary.