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

APP-6182 validate model triplets in meta.json #4369

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

abe-winter
Copy link
Member

@abe-winter abe-winter commented Sep 17, 2024

What changed

  • validate model triplets in meta.json before calling updateModels
    • this warns instead of erroring because this feels like something that could drift
  • factor out ParseAPI from API.UnmarshalJSON in resource package so it can be used in CLI

Why

  • give feedback to guard against errors people have hit

Reviewer question

  • should any of this logic be moved to the resource package within RDK? my gut says no because it would require more research

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Sep 17, 2024
@abe-winter abe-winter marked this pull request as ready for review September 17, 2024 14:07
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Sep 17, 2024
Copy link
Member

@zaporter-work zaporter-work left a comment

Choose a reason for hiding this comment

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

Nice! Really like the UX of also validating that the API is valid.

if err := api.Validate(); err != nil {
return errors.Wrap(err, "failed to validate API")
}
if !slices.Contains(rdkAPITypes, api.Type.Name) {
Copy link
Member

@zaporter-work zaporter-work Sep 17, 2024

Choose a reason for hiding this comment

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

[opt not a priority at all] use api.IsComponent() || api.IsService() like the other uses in rdk

Copy link
Member Author

Choose a reason for hiding this comment

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

if the rdkAPITypes slice lived in RDK instead of CLI, it would be a canonical place to store the check; it's self-updating in a way that the || expression isn't

Copy link
Member

Choose a reason for hiding this comment

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

Or better yet, if it lived inside api.Validate()

Copy link
Member Author

Choose a reason for hiding this comment

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

or like generally would be nice if go enums exposed the entire list of values at the language level

we sort of get this from the linter but only in switch statements

Copy link
Member Author

Choose a reason for hiding this comment

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

if it lived inside api.Validate()

yes good point

Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM; thanks!

@abe-winter abe-winter merged commit 909d47b into viamrobotics:main Sep 17, 2024
19 checks passed
@abe-winter abe-winter deleted the error-unk-apis branch September 17, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants