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

Patch Envelopes: Flatten ZInfoPatchEnvelopeUsage and remove opaque status #33

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

uncleDecart
Copy link
Member

Before submitting PR with Patch Envelopes status I'm opening one for EVE API to flatten ZInfoPatchEnvelopeUsage and remove Opaque Status since it's duplication of AppInstMetadata

Since this slightly changes API @gkodali-zededa @udit-zededa please review and comment :)

uint64 patchApiCallCount = 3;
// count the number of times app instance actually downloaded
// whole patch envelope or part of it
uint64 timesDownload = 4;
Copy link
Member Author

Choose a reason for hiding this comment

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

Apart from breaking changes, yetus suggests to change this to snake case. I didn't know that we are using snake case in proto messages

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we are using snake case in new protobuf (unless it is new values in an existing enum when we follow the existing convention for that enum).

The protoc produces the same output i.e., lowerCamelCase even as you change the proto file to use lower_snake_case.

Comment on lines 55 to 58
uint64 patchApiCallCount = 3;
// count the number of times app instance actually downloaded
// whole patch envelope or part of it
uint64 timesDownload = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of them have the semantics of "counter", but the naming convention does not tell that at all - "times" vs. "count". Can we make the names indicate that both are counters?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to snake_case with count in name

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Some items to fix, See separate comments.

…tatus

There is no need for additional structure since there is
1 to 1 releation. This commit also removes opaque status since it is
duplication of AppInstMetaData

Signed-off-by: Pavel Abramov <[email protected]>
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknordmark eriknordmark merged commit 3e88f72 into lf-edge:main Nov 1, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants