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

Support validation again snapshot - consistent order of reporter output #550

Merged
merged 5 commits into from
Feb 20, 2024

Conversation

sheidkamp
Copy link
Contributor

@sheidkamp sheidkamp commented Feb 5, 2024

Description

  • Updated all Validation functions to return errors and warnings in a consistent order
  • Added tests

Context:

ResourceReports is defined as type ResourceReports map[resources.InputResource]Report

We want to present the results of iterating over these reports in a consistent manner, and the difficulty is that while the InputResources may have unique instantiations, their underlying data might be identical.

The existing implementation does not guarantee any order, so updating to use a specific order will be backwards compatible.

Details:

The approach taken to ensure output consistency is:

  • initialize 2 data structures:
    • refKeys []string - a slice of the unique keys
    • refMap = map[string][]resources.InputResource - a map of the keys to the InputResources that are identified by that key.
  • loop over the ResourceReports and for each key res, which is an InputResource:
    • resKey := res.GetMetadata().Ref().String() as the key for the resource
    • add the resKey to refKeys
    • add the resource res to the slice of resources at refMap[resKey]
  • Sort the refKeys array

We can now loop over refKeys and access the resources in a consistent order. We still need to handle the ordering of resources that share a key, and we will do that in the individual validation functions.

In the validation functions:

  • loop over the sorted keys in refKeys
  • for each key, loop over the associated resources
    • for each resource, look up the report and append the errors/warnings to a slice
    • after processing the reports for a given key, there will be a slice of errors
      • this is a 1d slice, as the errors from each report will be contained in a single multierror
    • sort this array
    • append the elements of the sorted array of errors to the running error.

By executing this process, we can say for a given set of reports:

  • The resources will be iterated over in groups ordered by res.GetMetadata().Ref().String()
  • Within the groups of the resources the sets of errors will sorted
  • Because of this the sorting will be consistent each time Validation is run

Interesting decisions

There is an existing idiosyncrasy of the error reporting where an "invalid resource" error is added only the first resource encountered with an error:

errs = errors.Errorf("invalid resource %v.%v", res.GetMetadata().Namespace, res.GetMetadata().Name)
. This means if there are multiple resources with errors, only one will generate an "invalid resource" error.

This behavior has been maintained in the interest of rapid development

@sheidkamp sheidkamp changed the title Validate reports separate warnings Support validation again snapshot - separate warnings and consistent order of output Feb 5, 2024
@solo-changelog-bot
Copy link

Issues linked to changelog:
solo-io/gloo#8931

@sheidkamp sheidkamp force-pushed the validate-reports-separate-warnings branch from d097fcf to 70ca0a1 Compare February 5, 2024 19:16
davidjumani
davidjumani previously approved these changes Feb 6, 2024
Copy link
Contributor

@davidjumani davidjumani left a comment

Choose a reason for hiding this comment

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

Couple of nits. Apart from that LGTM

pkg/api/v2/reporter/reporter.go Show resolved Hide resolved
pkg/api/v2/reporter/reporter.go Show resolved Hide resolved
@sheidkamp sheidkamp changed the title Support validation again snapshot - separate warnings and consistent order of output Support validation again snapshot - consistent order of reporter output Feb 16, 2024
davidjumani
davidjumani previously approved these changes Feb 20, 2024
Copy link
Contributor

@davidjumani davidjumani left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

pkg/api/v2/reporter/reporter.go Outdated Show resolved Hide resolved
@soloio-bulldozer soloio-bulldozer bot merged commit 5693efe into main Feb 20, 2024
2 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the validate-reports-separate-warnings branch February 20, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants