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

refactor(flipt/validate): build entire snapshot on validate #2093

Merged
merged 8 commits into from
Sep 13, 2023

Conversation

GeorgeMac
Copy link
Member

@GeorgeMac GeorgeMac commented Sep 8, 2023

Fixes #2086
Fixes: FLI-568

This is a draft while we consider the proposed change.
The outstanding change needed to deliver this is a decision around referential integrity error enforcement.

This PR changes a few things, however, primarily it builds an entire store snapshot on flipt validate.
Doing this means we can get full confidence that a snapshot can be built from a target set of configuration on validate.
This give full confidence during e.g. a CI step.

To achieve this however, I have actually added the CUE validate step to the snapshot builder itself.
Meaning now, failures to validate via the CUE schema will fail snapshot building at runtime.
(This never happened before and the snapshot builder could be more lenient than the CUE schema).

When I did this, I found a couple constraints that I laxed in the existing CUE schema:

  1. Variant name is not required
  2. Percentage on a threshold can be represented as either an int or float in config

So, merging this, we might be introducing more stringent validation to the FS backends at runtime.
That said, the parity between the snapshot and building should mean that flipt validate is all the confidence you need.

Beyond this, this also update flipt validate so that it can take zero or more paths.
Zero paths used to mean, validate nothing.
Now it means, perform the same process as the FS backends. Meaning, attempt to parse the flipt index file and find all the files that match it (defaulting to the features.yaml files we default to now).

flipt validate can still be called with existing files, and only those files will be validated.

Outstanding Decision

Currently, the fs backend snapshot builder will just fail silently when references cant be resolved.
For example:

variant, found := findByKey(d.VariantKey, flag.Variants...)
if !found {
continue
}

We need to decide how to proceed here, in order to surface these as errors at validation time.
Depending on the decision, this may be considered a breaking change.

  1. We could just return an error here

This would be the breaking change for anyone currently just relying on the silent dropping of unreferenceable resources.

  1. We could warn instead, and optionally fail based on config

This idea is to add a warning log to these sites.
Then we could add a flag to both Flipts backend and flipt validate that could cause Flipt to fail fast in these situations.
For example, --strict.

This is obviously something that might be easy to miss, so has to be considered.
Maybe it is safe right now to just cut over and break this behaviour.

Update:

We chose to move forward with the breaking change.
So this PR introduces an error when a distributions variant cannot be resolved (as it does not exist).

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #2093 (93d6296) into main (29d3f9d) will decrease coverage by 0.06%.
The diff coverage is 51.75%.

@@            Coverage Diff             @@
##             main    #2093      +/-   ##
==========================================
- Coverage   70.81%   70.76%   -0.06%     
==========================================
  Files          73       73              
  Lines        7011     7042      +31     
==========================================
+ Hits         4965     4983      +18     
- Misses       1761     1774      +13     
  Partials      285      285              
Files Changed Coverage Δ
internal/cue/validate.go 62.06% <32.00%> (-23.98%) ⬇️
internal/storage/fs/snapshot.go 69.33% <47.14%> (+1.24%) ⬆️
internal/storage/fs/sync.go 86.02% <94.11%> (ø)
internal/storage/fs/store.go 79.31% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yquansah
Copy link
Contributor

yquansah commented Sep 8, 2023

@GeorgeMac I think a warning log for what we fail silently for makes sense for now so its not a breaking change. Maybe we can have a deprecation path forward that would make strict mode the default in the future.

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

im good with making this a somewhat breaking change tbh

@GeorgeMac GeorgeMac marked this pull request as ready for review September 11, 2023 16:06
@GeorgeMac GeorgeMac requested a review from a team as a code owner September 11, 2023 16:06
@GeorgeMac GeorgeMac merged commit c8d71ad into main Sep 13, 2023
23 of 25 checks passed
@GeorgeMac GeorgeMac deleted the gm/flipt-validate-build-snapshot branch September 13, 2023 12:57
markphelps added a commit that referenced this pull request Sep 13, 2023
* 'main' of https://github.com/flipt-io/flipt:
  refactor(flipt/validate): build entire snapshot on validate (#2093)
markphelps added a commit that referenced this pull request Sep 13, 2023
* main:
  refactor(flipt/validate): build entire snapshot on validate (#2093)
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.

flipt import reports errors that flipt validate does not
3 participants