Skip to content

Add proto for analysis messages #1179

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

Closed
wants to merge 3 commits into from
Closed

Conversation

zerobfd
Copy link
Contributor

@zerobfd zerobfd commented Nov 22, 2019

See https://docs.google.com/document/d/1lCnDBVzRzkr22BWjzPtGk6EK6EvGOnVIe2qBpaiQ3SM/ for more context and discussion around the change.

I split the struct up a bit differently than the doc to make things (hopefully) more clear and make the developer UX a bit easier.

@zerobfd zerobfd requested a review from ayj November 22, 2019 22:56
@zerobfd zerobfd requested a review from a team as a code owner November 22, 2019 22:56
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 22, 2019
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 22, 2019
@zerobfd
Copy link
Contributor Author

zerobfd commented Nov 25, 2019

/test build_api

@geeknoid
Copy link
Contributor

geeknoid commented Dec 2, 2019

There's a few things missing here.

#1. This new proto isn't being built. It needs to be built and linted. So it needs to be integrated into the make system.

#2. If you want this content to be documented on istio.io, then you need to add the needed voodoo in the makefile in order to produce a .pb.html file. If you do not intend to document this on istio.io, then you should remove the $ directives in the comments at the top of the file, since those are only relevant for the doc publishing process.

#3. Since I believe this proto will be used to describe status in Istio CRDs, then this probably will need to be integrated with the build of the .json files that described our schema to the API server. And will probably need to be integrated with the client-go libraries in order to enable programmatic access to the status state. @jasonwzm can help with the JSON stuff, @richardwxn can help with the client-go stuff.

@zerobfd zerobfd requested a review from a team as a code owner December 2, 2019 22:30
@zerobfd zerobfd force-pushed the messages-api branch 4 times, most recently from 38c0626 to 4280229 Compare December 3, 2019 18:43
@zerobfd
Copy link
Contributor Author

zerobfd commented Dec 4, 2019

/retest

1 similar comment
@zerobfd
Copy link
Contributor Author

zerobfd commented Dec 4, 2019

/retest

@jasonwzm
Copy link
Member

jasonwzm commented Dec 9, 2019

@zerobfd Can you rebase again to make gencheck work?

// Any message-type specific arguments that need to get codified. Optional.
google.protobuf.Struct args = 2;

// A list of strings specifying the path for resources which were the cause of
Copy link
Contributor

Choose a reason for hiding this comment

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

can you clarify the meaning of 'path for resources' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended this to mean the "kind/namespace/resource" construction which identifies a resource, but then someone told me that wasn't a path, it was a qualified resource name. Although when I google around I don't see anything about that--is there a concept of a resource ID somewhere that I can link to?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants