✨ Add have jsonpath matcher to komega

[matt-simons]

This PR adds a new matcher to Komega.

It's intended to be used in conjunction with the Object and ObjectList helper functions, transforming the actual based on the jsonpath provided. It works similarly to the HaveField matcher however has all the familiar benefits of using a JSON path. E.g. {.status.conditions[?(@.type=="Ready")]}.

When the JSON path resolves to a single result the result slice is flattened to a single value for convenience. E.g. HaveJSONPath("{.metadata.name}", Equal("foo")).

This is my first time contributing so apologies if I'm missing anything or not following any processes correctly :)

[k8s-ci-robot]

Welcome @matt-simons!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: matt-simons
Once this PR has been reviewed and has the lgtm label, please assign alvaroaleman for approval by writing /assign @alvaroaleman in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/envtest/komega/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @matt-simons. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[JoelSpeed]

Hey, thanks for proposing this, looks pretty interesting. I'm wondering, have you looked at proposing something like this directly to Gomega? I feel like it could be a useful addition in the wider gomega testing world as well

It might be useful if you could compare for us the usage of this, versus some vanilla gomega to see how much simpler this makes things, some concrete examples of usage would be great

I'm curious what happens if the json path validation fails, is that an error, or a failure in the code?

[matt-simons]

Hi Joel,

I hadn't considered contributing it to Gomega itself, I suppose there's no reason why it wouldn't work outside of a Kubernetes use case. The only k8s specific part is getting the UnstructuredContent. I'll reach out to the project to see if there's interest.

I'll make an example repo with my fork.

Currently if the json path validation fails that is an error, in my mind that would typically mean there's an issue with the test itself as most fields are strongly typed unless it was running against something like ConfigMap data. In that case using a KeyWithValue matcher would always be safe.

[vincepri]

/ok-to-test

[matt-simons]

I've created a repo to highlight the benefits here: https://github.com/matt-simons/havejsonpath-example

To summarise the convenience gain looks a bit like this:

Without HaveJSONPath:

id := func(element interface{}) string {
	return string(element.(appsv1.DeploymentCondition).Type)
}
Eventually(k.Object(deployment)).Should(HaveField("Status.Conditions",
	MatchElements(id, IgnoreExtras, Elements{
		"Available": MatchFields(IgnoreExtras, Fields{
			"Reason": Equal("MinimumReplicasAvailable"),
		}),
	}),
))

With HaveJSONPath:

Eventually(k.Object(deployment)).Should(HaveJSONPath(
	`{.status.conditions[?(@.type=="Available")].reason}`, Equal("MinimumReplicasAvailable")),
)

[JoelSpeed]

Just as another example, you could achieve the same with

Eventually(k.Object(deployment)).Should(HaveField("Status.Conditions",
	ContainElement(SatisfyAll(
		HaveField("Type", Equal("Available")), // Find the condition of type Available
		HaveField("Reason", Equal("MinimumReplicasAvailable")), // Check the Reason
	)),
))

If this were to fail because the list of conditions doesn't contain a condition of type Available, it will print out the entire conditions object, as the failing matcher is ContainElement and the input to that is the list of conditions, what would happen in the case of the HaveJSONPath?

[matt-simons]

That's a much nicer way of writing it.

In this example the HaveJSONPath failure message after timing out would look something like this:

  [FAILED] A spec timeout occurred and then the following failure was recorded in the timedout node before it exited:
  Timed out after 60.000s.
  The function passed to Eventually returned the following error:
  client rate limiter Wait returned an error: context canceled
      <*fmt.wrapError | 0xc0000a3560>: {
          msg: "client rate limiter Wait returned an error: context canceled",
          err: <*errors.errorString | 0xc0000982b0>{
              s: "context canceled",
          },
      }
  At one point, however, the function did return successfully.
  Yet, Eventually failed because the matcher was not satisfied:
  *v1.Deployment default/my-deployment at {.status.conditions[?(@.type=="NotAConditionType")].reason}: Expected
      <[]interface {} | len:0, cap:0>: []
  to equal
      <string>: MinimumReplicasAvailable

It doesn't list out the other conditions we weren't looking for and states that the default/my-deployment at {.status.conditions[?(@.type=="NotAConditionType")].reason} resolved to nothing

[JoelSpeed]

I think we should follow the conversation on the gomega issue and see what the gomega maintainers think of the proposal, my suspicion is that they will suggested to use something like my previous example, as that gives you more specific feedback about the failure, but then, we are looking at only one example, more examples my help to show the value of the new matcher

[schrej]

Are you aware of the EqualObject matcher that's also part of the komega package? There is some overlap between what you're proposing here and the already existing matcher.

The main thing I dislike about this solution is that it's using a different format than the allow/ignore path descriptions that can be used with EqualObject. The idea there was to make it as effortless to use as possible, allowing both struct names as well as json names to be used. Of course using proper JSON paths is a lot more powerful, but I'm not sure whether that's necessary here.
EqualObject currently requires to use indices when matching arrays. Maybe that is inconvenient in some cases, especially since the order of elements might be nondeterministic. With the current implementation it's hard (iirc) to improve that and allow e.g. finding a specific element by key/value pair. On the other hand I'm not sure whether it's worth the effort when options like the ones that were pointed out already exist.

Since your approach is about matching single values, we could also think about a HaveObjectPath that is similar to HaveField, but allows to use both struct field names as well as json, making it similar to EqualObject in usage.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.