Skip to content
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

rfc: configuration and shared files #98

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JP-Ellis
Copy link
Contributor

RFC proposal to try and standardise the way we configure and share files across the Pact ecosystem.

Rendered view can be found here.

Excerpt from the RFC:

Summary

This RFC proposes to establish a standard way to configure and share common files across the Pact ecosystem, including the Pact Broker, PactFlow, and the various Pact implementations.

Motivation

The Pact ecosystem is quite diverse and we would like to ensure that transitioning between different tools is as seamless as possible. Several de-facto standards have emerged, but these have not been formalised. Furthermore, there is no clear way to extend these de-facto standards to new tools or use cases.

This RFC aims to formalise some existing de-facto standards. Furthermore, this RFC proposes new standards with the hope of ensuring a more consistent experience across the Pact ecosystem and into the future.

This RFC is split into three main categories:

  • Environment Variables: Define a standard way to configure Pact implementations using environment variables.
  • Configuration: Define a standard way to store runtime configuration for Pact implementations.
  • Shared Files: Define a standard way to store shared files across the Pact ecosystem.

@JP-Ellis JP-Ellis self-assigned this Aug 30, 2024
@JP-Ellis JP-Ellis force-pushed the rfc/configuration-and-shared-files branch from 48c1097 to 37b9d3f Compare September 3, 2024 04:37
@YOU54F
Copy link
Member

YOU54F commented Sep 3, 2024

Looks good to me. I assume for most implementors of client languages, this will be handled by the FFI, and other consumers would be the rust and ruby suite of CLI tools?

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented Sep 4, 2024

Updated the RFC with a few changes:

  • Clarifications around the following points:
    • Precedence of CLI arguments
    • Precedence of explicitly set Pact configuration files
    • When warning should not be raised if the fallback location is used

When updating these, I also identified a gap in the way the TOML configuration ought to handle paths. So I have added a subsection to the configuration section to make this clear.

Paths can be a source of confusion/annoyances especially when sharing across
operating systems. Therefore, explicit requirements are set.

Signed-off-by: JP-Ellis <[email protected]>
@JP-Ellis JP-Ellis force-pushed the rfc/configuration-and-shared-files branch from 6a3cc05 to e6d181e Compare September 4, 2024 00:52
- **Configuration**: Define a standard way to store runtime configuration for Pact implementations.
- **Shared Files**: Define a standard way to store shared files across the Pact ecosystem.

## Environment Variables
Copy link
Contributor

@tienvx tienvx Sep 5, 2024

Choose a reason for hiding this comment

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

Language level:

Pact-PHP allow using PACT_LOGLEVEL to configure log level for FFI library.

I'm not sure if other implementations has this ability, and if they have, should we use this variable name across implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logging is, in my experience, one of these things which can vary significantly based on the language's convention. So I don't know if that is something we want to fix in an RFC.

If we did implement this, my suggestion would be:

  • Have PACT_LOG as the environment variable name, or have it stored at:

    [pact]
    log = "..."
    

    in the config TOML.

  • Have the values be either of the following (case insensitive):

    • DEBUG
    • INFO
    • WARN or WARNING
    • ERROR

But like I said, I am hesitant. Logging can be complex, and I do think it is best to fit in with the conventions of the language.

Copy link
Contributor

@tienvx tienvx Sep 19, 2024

Choose a reason for hiding this comment

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

I'm fine with leaving this for future RFC. We can resolve this one.

[pact]
plugin_dir = "~/.local/share/pact/plugins"
data_home = "~/.local/share/pact"
do_not_track = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like:

  • log_level = debug or
  • loglevel = debug

is important and can be included in this file.

But like PACT_LOGLEVEL env, I'm not sure how this config/env behave in different pact's implementations.


- `plugin_dir`: The directory where shared plugins are stored. Equivalent to `PACT_PLUGIN_DIR`.
- `data_home`: The directory where shared files are stored. Equivalent to `PACT_DATA_HOME`.
- `do_not_track`: If set to `true`, the Pact implementation _must not_ report any usage analytics data back to the Pact Foundation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention the default value is false (opt-out) ?


- `plugin_dir`: The directory where shared plugins are stored. Equivalent to `PACT_PLUGIN_DIR`.
- `data_home`: The directory where shared files are stored. Equivalent to `PACT_DATA_HOME`.
- `do_not_track`: If set to `true`, the Pact implementation _must not_ report any usage analytics data back to the Pact Foundation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Miss the equivalent part: Equivalent to PACT_DO_NOT_TRACK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update!

- `base_url`: The base URL of the Pact Broker. Equivalent to `PACT_BROKER_BASE_URL`.
- `username`: The username to use when authenticating with the Pact Broker. Equivalent to `PACT_BROKER_USERNAME`.
- `password`: The password to use when authenticating with the Pact Broker. Equivalent to `PACT_BROKER_PASSWORD`.
- `token`: The token to use when authenticating with the Pact Broker. Equivalent to `PACT_BROKER_TOKEN`.
Copy link
Contributor

@tienvx tienvx Sep 5, 2024

Choose a reason for hiding this comment

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

I'm not sure, that if username/password is set then token is ignored and vice versa?

Should we mention that if it's true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When talking about the environment variables, I do state that having a mix of username/password with token is undefined behaviour.

Having said that, this does bring a good question as to how best to handle a local override to a user config; as this could quite easily result in all three being defined.

To resolve this, I actually think it might be best to change the TOML config to:

[broker]
auth = { token = "..." }
# or
auth = { username = "...", password = "..." }

rfc/0000-configuration-and-shared-files.md Show resolved Hide resolved
At present, there are only a few files which are shared across the Pact ecosystem. These are:

- Plugins, currently stored in `~/.pact/plugins`
- The Pact core library (i.e., the Pact FFI), though only in some implementations.
Copy link
Contributor

@tienvx tienvx Sep 5, 2024

Choose a reason for hiding this comment

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

what are proposed paths? e.g.

  • ~/.local/share/pact/ffi/pact.h
  • ~/.local/share/pact/ffi/pact.so
  • ~/.local/share/pact/ffi/pact-stub-server
  • fallback
    • ~/.pact/ffi/pact.h
    • ~/.pact/ffi/pact.so
    • ~/.pact/ffi/pact-stub-server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually specify how the FFI binaries/headers should be stored, mostly because I don't know what practices are used across the ecosystem.

Having $PACT_DATA_HOME/ffi/pact.h and $PACT_DATA_HOME/ffi/(lib)pact.(ext) seems quite reasonable to me though.

This assumes it is dynamically linked though, which isn't the case for all implementations.

Copy link
Member

Choose a reason for hiding this comment

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

We might need to think about versioning here. For instance, a user might be using Pact JS and Pact Go. It might be OK to have a latter version of the FFI for one and not the other (e.g. they have just started using Pact Go, and are on the latest version, but Pact JS has an existing suite and they are on an older version). In theory, this should only be a problem for major versions of the FFI, but in practice this isn't always the case.

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps a way to address it is by convention.

e.g.

  • [base path]/ffi/pact.h
  • [base path]/ffi/pact.so
  • [base path]/ffi/pact-js/pact.h
  • [base path]/ffi/pact-js/pact.so
  • [base path]/ffi/pact-go/pact.h
  • [base path]/ffi/pact-go/pact.so

Pact JS and Pact Go would have separate versions, and all other libraries use the fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually thinking more about this, and I was wondering whether we should recommend the standard Unix way of organising these files:

$PACT_DATA_HOME/include/pact.h
$PACT_DATA_HOME/lib/libpact.so

and then libraries can opt in to library-specific version of the FFI with

$PACT_DATA_HOME/include/pact.js.h
$PACT_DATA_HOME/lib/libpact.js.so
$PACT_DATA_HOME/include/pact.go.h
$PACT_DATA_HOME/lib/libpact.go.so
...

This follows the same pattern of having multiple version of libraries presented as:

lib/libfoo.so      -> libfoo.2.so
lib/libfoo.2.so    -> libfoo.2.1.so
lib/libfoo.2.1.so
lib/libfoo.1.so    -> libfoo.1.8.so
lib/libfoo.1.8.so

(the arrows are symlinks, but we need not use symlinks here)

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented Oct 28, 2024

Plan for accepting RFC

This RFC has been open for a while, and has received little additional feedback for a while, with the general consensus so far being in favour of this change.

I have a partial implementation working privately, which I will spin out into a dedicated Rust crate once this RFC is merged.

I am making some suggestions below, and therefore would like to leave this open for at least another 2-3 weeks to give everyone the opportunity to give more feedback.

Amendment for FFI header/lib path

I would like to take this opportunity to add a standard location for the Pact library (should we use dynamic linking). Some discussion was started in this conversation.

I think this RFC should be amended with the following change:


A new optional section will be added to the config containing two values:

[pact.ffi]
include_path = "..."
lib_path = "..."

These values can also be set through the environment variables

  • PACT_FFI_INCLUDE_PATH
  • PACT_FFI_LIBRARY_PATH

Should they be unset, they should default to:

  • Searching $PACT_DATA_HOME/include and $PACT_DATA_HOME/lib
  • Searching the system paths (C_INCLUDE_PATH and LIBRARY_PATH and/or LD_LIBRARY_PATH respectively).

Support for multiple paths is allowed by using a colon-separated environment variable, or using a list in TOML. If multiple paths are specified, then each path is tried in order.

Suffixes (home, dir, path, etc.)

One last thing while re-reading the RFC that I noticed is that there is little consistency between _path or _home or _dir; or using _file or not.

The only precedent we have at the moment in the Pact ecosystem is the PACT_PLUGIN_DIR environment variable.

Here are my thoughts:

  • Anything that should point to a specific file should end in _FILE to avoid confusion with a directory or an option.
  • Anything that should point to a specific directory should end in either _DIR or _DIRECTORY to avoid confusion with a file or an option
  • Anything that points to a collection of directories (including a collection of one directory) should end in _PATH

This follows my experience with environment variables from using other tools on Linux/Unix. This change would change the PACT_DATA_HOME environment variable to PACT_DATA_DIR for example.

Please let me know if you think this is something worth adding to the RFC, or whether this is an unnecessary complication.

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.

4 participants