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

Make the config versioning/compatibility independent of the butido version and improve the config checks #334

Conversation

primeos-work
Copy link
Member

@primeos-work primeos-work commented Jan 8, 2024

tl;dr:

  • Version the butido configuration with integer numbers (vs. SemVer version requirements)
  • Check if the config is compatible before trying to load (type check) it to improve the error messages
  • Automatically output the required configuration changes (changelog) if updates are required
  • Add tests for the example config files in this repo
  • Fail with an error if there are unknown settings in the configuration
  • Improve the checks for missing directories (one missing check + two wrong config key names)

Note: This makes breaking configuration changes that require users to update their configuration (should be trivial though as one only has to set compatibility=1 and remove invalid settings, if present).

Example outputs

Before this PR:

$ butido build -I rh8 tree 1.8.0
Error: Failed to validate configuration

Caused by:
    Configuration is not compatible to butido 0.4.0

If config upgrades are required:

$ butido build -I rh8 tree 1.8.0
The butido configuration is too old and the following changes are required:
- Version 1: The format of the `compatibility` setting has changed from a string (`semver::VersionReq`) to a number (`u16`).
- Update the `compatibility` setting to `1`

Error: The butido configuration failed the compatibility check

Caused by:
    0: The expected configuration version is 1 while the provided configuration has a compatibility setting of 0
    1: The provided configuration is not compatible with this butido binary

Trying to load an old configuration:

$ butido build -I rh8 tree 1.8.0
Error: The butido configuration failed the compatibility check

Caused by:
    0: Set "compatibility" to 0 to get a summary of the required changes
    1: The format of the "compatibility" setting has changed from a string to a number
    2: Failed to parse the value of the compatibility setting (0.4.0) into a number (str -> u16)
    3: invalid digit found in string

Loading a configuration with invalid settings:

$ butido build -I rh8 tree 1.8.0
Error: Failed to load (type check) the butido configuration

Caused by:
    unknown field `invalid`, expected one of `compatibility`, `log_dir`, `strict_script_interpolation`, `progress_format`, `spinner_format`, `package_print_format`, `build_error_lines`, `script_highlight_theme`, `script_linter`, `shebang`, `releases_root`, `release_stores`, `staging`, `source_cache`, `database_host`, `database_port`, `database_user`, `database_password`, `database_name`, `database_connection_timeout`, `docker`, `containers`, `available_phases`

Invalid env vars:

$ BUTIDO_TEST=42 butido build -I rh8 tree 1.8.0
Error: Failed to load (type check) the butido configuration

Caused by:
    unknown field `test`, expected one of `compatibility`, `log_dir`, `strict_script_interpolation`, `progress_format`, `spinner_format`, `package_print_format`, `build_error_lines`, `script_highlight_theme`, `script_linter`, `shebang`, `releases_root`, `release_stores`, `staging`, `source_cache`, `database_host`, `database_port`, `database_user`, `database_password`, `database_name`, `database_connection_timeout`, `docker`, `containers`, `available_phases`

Ideally, the butido version should be bumped when there are breaking
changes but this new approach offers the following advantages:
- The compatibility setting doesn't need to be changed when we release a
  new butido version (e.g., with breaking changes that are unrelated to
  the configuration).
- An independent configuration version is more meaningful: We can update
  it immediately when we add a breaking change (no need to make a new
  release), inform the user of the necessary changes, and don't "have"
  to bundle breaking changes.
- One minor issue was that, e.g., configuration version 0.4.1 wouldn't
  be compatible with butido 0.4.0 (but that can be avoided by always
  setting `compatibility` to `x.y.0`, which is compatible to butido
  version `x.y.*`).
- The new type is stricter (`semver::VersionReq` is a SemVer version
  requirement and can be "*", ">=1.2.3, <1.8", etc.).

This fixes science-computing#187.

PS: We should also start collecting "upcoming" breaking/relevant changes
in the changelog so that we don't have to go through the commit history
(all of the commits since the last tag) when preparing a new release.

PPS: I chose to start with a `compatibility` setting of `1` so that we
could use `0` for a "smoother transition", if necessary.

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
Otherwise the config loading (type checking) will likely fail before we
can output the appropriate error message with the required changes.

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
This should make it much easier to update butido's configuration, if
required after a butido update due to breaking configuration changes.

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
This is important to avoid unnecessary runtime failures (those could
easily get missed as the relevant code is conditional).

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
It'd be embarrassing if the example config file wouldn't be
valid so let's better add a test for it.
Skip the filesystem checks (if the Butido directories exist) when
validating the example configuration for the test.

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
I did previously set `compatibility=0` as I expected that this
configuration file could easily become outdated (i.e., to signal it
isn't really up-to-date) but with this test we can finally ensure that
it'll remain valid :)

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
This helps catching errors/typos in butido's configuration as well as to
cleanup settings that don't exist anymore or got renamed.
It already revealed a misnamed key in our example configuration file
(that caused the corresponding test to fail)!

We already implemented this for package definitions in b075e97.

The error output contains the name of the unknown field and a list of
the valid/expected fields.

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
I already made a mistake (accidentally introduced a `!` - luckily it was
caught by CI for other reasons) in one of the three checks when
introducing `skip_filesystem_checks`.
This simplifies the code and ensures that all checks behave the same and
I already noticed another mistake that I'll fix next (not part of this
commit to keep it a refactoring).

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
Otherwise a missing log directory would result in an error at the end of
a build command (despite the build being successful).

Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
@primeos-work primeos-work force-pushed the improve-config-versioning-logic branch from f7cd656 to 34716b1 Compare January 8, 2024 18:55
@primeos-work primeos-work linked an issue Jan 10, 2024 that may be closed by this pull request
Copy link
Collaborator

@christophprokop christophprokop left a comment

Choose a reason for hiding this comment

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

Tested. Thank you!

@christophprokop christophprokop added this pull request to the merge queue Jan 11, 2024
Merged via the queue into science-computing:master with commit 8943e61 Jan 11, 2024
13 checks passed
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.

config.toml versioning decoupled from butido version
2 participants