Skip to content

[hooks] [hooks_runner] Make environment var filtering user-configurable #3396

@dcharkes

Description

@dcharkes

We should consider packages with hooks being able to define the environment variables they need. Otherwise we'll keep adding environment variables and env variable prefixes to the hooks_runner allowlist. (#3394, #3103, #3305, ...)

Some ideas:

1: Root workspace pubspec allowlist

We already have a user_defines key under hooks in the pubspec, so we can add it there

hooks:
  user_defines: # only the root package workspace pubspec.yaml are taken into account
    <package>:
      <key>: <value>
  environment_variables: # only the root package workspace are taken into account
    include:
      - NUGET_PACKAGES
    prefix:
      - NUGET_

Con:

  • End users ~1M might need to add env vars
  • yaml configs are not discoverable

2: Package pubspec allowlist

We could allow every package with hooks' pubspec.yaml to contain a hooks->environment_variables.

hooks:
  environment_variables: # only applies to the hooks of their own package _and_ the hooks of all dependent packages
    include:
      - NUGET_PACKAGES
    prefix:
      - NUGET_

We could then filter the env vars based on the global allowlist + the individual pack env vars per package in the hooks runner.

Note that we would also need to pass all the env vars of a packages' dependencies. E.g. if package:my_clib uses package:native_toolchain_c in its hooks and native_toolchain_c needs NUGET_ env var to function properly, we don't want to repeat that in package:my_clib.

Pro:

  • Only package authors have to define it

Con:

  • Hooks_runner becomes slower because N pubspecs have to be read and parsed to find the env vars
  • yaml configs are not discoverable

3. hooks/config.yaml

environment_variables:
  include:
    - NUGET_PACKAGES
  prefix:
    - NUGET_

Instead of specifying it in the pubspec.yaml, we could introduce a new yaml, that way we don't spend extra time parsing things we don't need.

Pro:

  • Only package authors have to define it
  • Fast

Con:

  • yaml configs are not discoverable

4. Pass in the full env BUT require users to write the env vars they accessed for caching purposes

Pro:

  • No slower hooks_runner

Con:

  • Breaking change to the protocol. As all existing hooks do not declare their env-var use from Platform.environment

5. Add a HookInput.environmentUnfiltered

The full environment would be written to a JSON file and passed into the hook (so not as Platform.environment). And the API accessing environmentUnfiltered would implicitly record a dependency on that env key.

Pro:

  • No slower hooks_runner
  • Dart API is discoverable

Con:

  • Nothing prevents hooks-writers from iterating over the full environment

6. hook/configure.dart

We could have a hook/configure.dart (#3027) before build and link hooks that could specify in its output what env vars the build and link hooks get.

Pro:

  • Dart API is discoverable

Con:

  • Slow, running a hook is slower than reading a yaml file
  • Wouldn't the configure hook need configuration for what env vars it needs? 😄

I think I'm leaning slightly towards option 3. I don't like that yaml files are not discoverable, but a Dart API is hard if we're configuring with what environment the Dart file with the Dart API is invoked.

cc @goderbauer @mraleph @liamappelbe If you have any other bright ideas, let me know.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type
    No fields configured for issues without a type.

    Projects

    Status

    No status

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions