Upgrade conftest to v0.62.0 (after opa v1.0 upgrade) #2656

simonbaird · 2025-07-24T22:10:12Z

Ref: https://issues.redhat.com/browse/EC-1408

simonbaird · 2025-08-05T13:08:27Z

The acceptance tests passed for me locally, so not sure why they failed in GitHub. Will try running them again.

Acepresso · 2025-08-06T13:02:58Z

LGTM

simonbaird · 2025-08-06T18:43:33Z

I reworked the "Provide data source dir list.." commit so it correctly handles data inside nested subdirectories.

codecov · 2025-08-06T18:49:45Z

Codecov Report

❌ Patch coverage is 45.38462% with 71 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/test/test.go	0.00%	53 Missing ⚠️
internal/evaluator/conftest_evaluator.go	79.45%	15 Missing ⚠️
internal/documentation/documentation.go	0.00%	2 Missing ⚠️
cmd/root.go	0.00%	1 Missing ⚠️

Flag	Coverage Δ
generative	`?`
integration	`66.79% <45.38%> (+0.21%)`	⬆️
unit	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/initialize/init_policies.go	`91.04% <100.00%> (ø)`
internal/policy/source/source.go	`83.59% <ø> (ø)`
cmd/root.go	`0.00% <0.00%> (ø)`
internal/documentation/documentation.go	`0.00% <0.00%> (ø)`
internal/evaluator/conftest_evaluator.go	`81.56% <79.45%> (-0.14%)`	⬇️
cmd/test/test.go	`0.00% <0.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

simonbaird · 2025-08-06T19:11:04Z

I don't particularly want to add test coverage for cmd/test/test.go here in this PR, so I might ignore the Codecov complaint.

simonbaird · 2025-08-06T21:58:49Z

I'm slightly worried about https://issues.redhat.com/browse/EC-1418 , so I might hold off merging this now.

st3penta · 2025-08-07T09:33:18Z

internal/evaluator/conftest_evaluator.go

+	dataDirs := []string{}
+	for dir := range dirsWithDataFiles {
+		dataDirs = append(dataDirs, dir)
+	}


do you know that with go 1.23 we have helper functions for this? not a big gain from the vanilla approach though 😄

Suggested change

dataDirs := []string{}

for dir := range dirsWithDataFiles {

dataDirs = append(dataDirs, dir)

}

dataDirs := slices.Collect(maps.Keys(dirsWithDataFiles))

Ooh, no I didn't. I really like it! For this PR I guess I wanna to stick with the old conventions for consistency with the rest of the code. Maybe we do a bigger refactor/cleanup in the near future and adopt the nice new style everywhere.

st3penta

lgtm

The motivation is to make sure PR conforma#2656 doesn't change this behavior and potentially break things. The two things being verified: - If the value of the top level key is a map, then the map keys from each data source get merged - If the value of the top level key is not a map, then we fail hard with an error. Ref: https://issues.redhat.com/browse/EC-1418

simonbaird · 2025-08-07T18:41:56Z

Setting to draft since I want to merge #2681 then rebase this on top of it.

simonbaird · 2025-08-07T19:54:47Z

Not sure why acceptance test failed in Sealights, but passed otherwise.. 🤔

The motivation is to make sure PR conforma#2656 doesn't change this behavior and potentially break things. The two things being verified: - If the value of the top level key is a map, then the map keys from each data source get merged - If the value of the top level key is not a map, then we fail hard with an error. Ref: https://issues.redhat.com/browse/EC-1418

Notably this is the first version of the conftest module that uses/supports the v1.x opa module. It required a little extra manual tweaking to remove the `replace muzzammil.xyz/jsonc ...` which is not needed now after open-policy-agent/conftest#1133 was merged. At this point I'm not sure exactly what the impact will be, but we have not updated this for a while. Now that we updated opa, let's update this also. Edit: There was quite a lot of impact actually, see the other commits in this PR. Ref: https://issues.redhat.com/browse/EC-1408 Ref: https://issues.redhat.com/browse/EC-1130

Looks like a refactor was done upstream related to how exit codes are handled. This updates our code to use the new methods and get builds working. Ref: https://issues.redhat.com/browse/EC-1408

Deal with an upstream refactor of the conftest module related to how capabilities and loaded. Also add the new RegoVersion "v1" attribute. Ref: https://issues.redhat.com/browse/EC-1408

As mentioned in the comments, it's not causing any specific harm, but for now it causes many snapshot comparisons to fail when running the tests. Let's remove it. Ref: https://issues.redhat.com/browse/EC-1408

The code in cmd/test/test.go is meant to be an "almost" copy of the conftest cmd/test/test.go. (It's only functional difference is to support one additional output format, the "appstudio" format). To update the file I used this helper to compare the upstream conftest file with our local copy: DIFF_TOOL=vimdiff make conftest-test-cmd-diff There are some flags changing here, so this could be a breaking change for users of `ec test`. As well as the cmd change, there is a snapshot adjustment due to a new metadata key that Conftest is producing. FWIW I don't think this is a good strategy long term. Maintaining this file manually is quite difficult. Maybe it's possible to do it a different way, e.g. the way it's done in cmd/opa, but still add support for the additional format. Also I'm not sure if anyone is using this experimental feature, so we could consider dropping it. (But let's worry about all that later.) Ref: https://issues.redhat.com/browse/EC-1408

We need to use the newer opa v1 syntax. This fixes a failing acceptance test in features/initialize.feature. Side-note: The `ec init policy` command is a bit undercooked, and I don't it's been used much. In this commit I'm just doing the minimum required to make the tests pass, and resisting the urge to also try to improve it. Ref: https://issues.redhat.com/browse/EC-1408

The timestamp part was removed a while ago in commit 1ca0a62 . Minor fixup while working on... Ref: https://issues.redhat.com/browse/EC-1408

I'm assuming there's no good reason to be ignoring that error. An incidental improvement while working on... Ref: https://issues.redhat.com/browse/EC-1408

..instead of just the top level data dir. After updating conftest, the way the data is passed into the rego evaluator somehow changed. (Actually I'm not sure how/where that happened, or whether it was due to a change in the underlying opa engine, or in the conftest wrapper.) The change is related to using data subdirectory names as keys in the data. So for example if you provide "./data" as the data directory, and there was a file there like "data/foo/bar/config.json" the foo and bar directory names become keys in the data structure visible to rego, e.g. "data.foo.bar.whatever". We don't want that, particularly because we place data from each data source in a subdirectory with a generated name based on a checksum of the source url. Also, for the trusted task data bundle, we end up with the actual yaml file under several levels of subdirectories, something like {{workdir}}/data/e8a615778/data/data/trusted_tekton_tasks.yml, which means we'd need to look for the trusted tasks in data.e8a615778.data.data.trusted_tasks[refkey] instead of data.trusted_tasks[refkey]. To avoid the directories becoming keys in the data we need to provide an explicit list of the data directories to the engine instead of just the one top level data directory. In the above example, we'd need to provide (effectively) `--data "data/foo/bar" instead of `--data "data"`. There's also a related change for the config.json to place it in "data/config/config.json" instead of at the top level, i.e. "data/config.json" so its location is consistent with the other data sources. Ref: https://issues.redhat.com/browse/EC-1408 Co-authored-by: Claude Sonnet 4

See also the explanations in the previous commit. This change adds some more visibly obvious test coverage for that behavior. If it wasn't for the changes in the previous commit, we'd see foo and bar become keys in the data and the test would fail. Ref: https://issues.redhat.com/browse/EC-1408

simonbaird · 2025-08-08T19:03:03Z

/retest

simonbaird · 2025-08-08T19:58:50Z

Looking at the Codecov report it seems to me mostly these:

+ 	if err != nil {
+ 		return
+ 	}

Getting coverage on those err handling code paths is very low value, so I'm not going to pay attention to the failure.

simonbaird · 2025-08-08T20:01:21Z

With the test coverage from #2681 I'm now pretty confident about this. Let's merge.

simonbaird changed the title ~~Conftest upgrade~~ Upgrade conftest to v0.62.0 (after big opa upgrade) Jul 24, 2025

simonbaird force-pushed the conftest-upgrade branch 2 times, most recently from 2cfc119 to 72c9c12 Compare August 5, 2025 13:07

simonbaird marked this pull request as ready for review August 5, 2025 13:24

simonbaird force-pushed the conftest-upgrade branch from 72c9c12 to 94b4a78 Compare August 5, 2025 16:07

Acepresso approved these changes Aug 6, 2025

View reviewed changes

simonbaird force-pushed the conftest-upgrade branch from 4d4bfbc to 3fe5863 Compare August 6, 2025 18:42

st3penta reviewed Aug 7, 2025

View reviewed changes

st3penta approved these changes Aug 7, 2025

View reviewed changes

simonbaird mentioned this pull request Aug 7, 2025

Add test coverage for top level data key merging #2681

Merged

simonbaird marked this pull request as draft August 7, 2025 18:41

simonbaird force-pushed the conftest-upgrade branch from 3fe5863 to eb4a166 Compare August 7, 2025 18:48

simonbaird added 7 commits August 8, 2025 12:20

Deal with exit code changes in conftest module

b0ebb3a

Looks like a refactor was done upstream related to how exit codes are handled. This updates our code to use the new methods and get builds working. Ref: https://issues.redhat.com/browse/EC-1408

Update capabilities handling for new conftest

9d6a311

Deal with an upstream refactor of the conftest module related to how capabilities and loaded. Also add the new RegoVersion "v1" attribute. Ref: https://issues.redhat.com/browse/EC-1408

Strip out new "query" Conftest metadata key

3a33e11

As mentioned in the comments, it's not causing any specific harm, but for now it causes many snapshot comparisons to fail when running the tests. Let's remove it. Ref: https://issues.redhat.com/browse/EC-1408

Fix incorrect comment about uniqueDir timestamp

6506f24

The timestamp part was removed a while ago in commit 1ca0a62 . Minor fixup while working on... Ref: https://issues.redhat.com/browse/EC-1408

simonbaird added 3 commits August 8, 2025 12:20

Stop ignoring potential create directory errors

a0eb6e3

I'm assuming there's no good reason to be ignoring that error. An incidental improvement while working on... Ref: https://issues.redhat.com/browse/EC-1408

simonbaird force-pushed the conftest-upgrade branch from eb4a166 to 73b94d2 Compare August 8, 2025 16:46

simonbaird changed the title ~~Upgrade conftest to v0.62.0 (after big opa upgrade)~~ Upgrade conftest to v0.62.0 (after opa v1.0 upgrade) Aug 8, 2025

simonbaird marked this pull request as ready for review August 8, 2025 19:56

simonbaird merged commit 38f45e6 into conforma:main Aug 8, 2025
12 of 13 checks passed

Upgrade conftest to v0.62.0 (after opa v1.0 upgrade) #2656

Upgrade conftest to v0.62.0 (after opa v1.0 upgrade) #2656

Uh oh!

Conversation

simonbaird commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonbaird commented Aug 5, 2025

Uh oh!

Acepresso commented Aug 6, 2025

Uh oh!

simonbaird commented Aug 6, 2025

Uh oh!

codecov bot commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

simonbaird commented Aug 6, 2025

Uh oh!

simonbaird commented Aug 6, 2025

Uh oh!

st3penta Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

simonbaird Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

st3penta left a comment

Choose a reason for hiding this comment

Uh oh!

simonbaird commented Aug 7, 2025

Uh oh!

simonbaird commented Aug 7, 2025

Uh oh!

simonbaird commented Aug 8, 2025

Uh oh!

simonbaird commented Aug 8, 2025

Uh oh!

simonbaird commented Aug 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

simonbaird commented Jul 24, 2025 •

edited

Loading

codecov bot commented Aug 6, 2025 •

edited

Loading