Add support for anyOf to skip_prompt_if

[arpitjasa-db]

Changes

This PR:
Introduces anyOf to skip_prompt_if. This allows you to make OR conditionals for skipping prompts during template initialization.

Tests

Added unit test and confirmed existing ones still work. Also tested manually.

[shreyas-goenka]

Thanks! Needs a minor refactor.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1b62417) 49.50% compared to head (2ed579a) 50.37%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1133      +/-   ##
==========================================
+ Coverage   49.50%   50.37%   +0.86%     
==========================================
  Files         281      288       +7     
  Lines       10713    10945     +232     
==========================================
+ Hits         5303     5513     +210     
- Misses       4846     4863      +17     
- Partials      564      569       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pietern]

The validateConst function expects s.Properties to be non-nil. This is not guaranteed (I think), looking at the JSON schema spec. I understand we're only interested in validating the full config object (i.e. a string/string map) for the skip-prompt-if functionality here, but this seems brittle.

Having the anyOf property in the schema invites folks to use it as the JSON spec intends (i.e. also at the single field level) and that would cause panics here.

[shreyas-goenka]

I don't think it'll panic right? Maybe I am missing what you are pointing to, but a single field anyOf will simply be ignored in the current implementation.

[pietern]

In the following (valid) example, the properties field is not set, and the code will panic.

{
  "anyOf": [
    {
      "type": "string"
    }
  ]
}

[shreyas-goenka]

Tried out setting anyOf like this, but it does not panic.

Accessing empty maps does not panic. You can try this out in the go playground. This does not panic:

	type apple struct {
		foo string
		bar map[string]any
	}

	fruit := apple{}
	v, ok := fruit.bar["key"]
	fmt.Printf("v: %v, ok: %v\n", v, ok)

[pietern]

Thanks, TIL!

Looked into the spec and it says:

A nil map is equivalent to an empty map except that no elements may be added.

[shreyas-goenka]

Nice! Did not know this :)

[pietern]

Why do all these tests run both validateConst and ValidateInstance? Looks very repetitive.

[pietern]

This follows the pattern used in the rest of the file, so no need to address in this PR.

Could you take a look at this though?

[shreyas-goenka]

That was intentional. At the cost of being very repetitive, we ensure that the main public API exposed ie ValidateInstance and the specific validation rule validateConst are correct. This works here because validations in JSON schema are additive.

Are there alternative approaches you would recommend here? Maybe only testing ValidateInstance or validateConst?

[pietern]

Does testing the unexported function add fidelity to the test?

[shreyas-goenka]

Not sure what fidelity means in this context, but testing the unexported func does provide information about where the error comes from. Each unexported function here maps to a rule in JSON schema validation (const vs anyOf for example), and confirming the following seems helpful to me:

1. Error originated from the JSON schema rule we expected it to.
2. Other rules (unexported functions) did not conflict with this one.

[shreyas-goenka]

I can see the flip side however that unexported functions are not part of the public API. We are good to go as long as the public API return's the right errors.
It does make maintain this easier.

Happy to remove testing for the unexported function.