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.

Sign up for GitHub

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

Jump to bottom

public-api/viz split 1/8: a couple more protobuf changes #5553

Closed
wants to merge 1 commit into from
Closed

public-api/viz split 1/8: a couple more protobuf changes #5553

wants to merge 1 commit into from

Conversation

alpeb
Copy link
Member

@alpeb alpeb commented Jan 18, 2021

  • Moved healthcheck.proto back from viz to proto/common as it remains being used by the main healthcheck.go library (it was moved to viz by Separate observability API #5510).
  • Extracted from viz.proto the IP-related types and put them in /controller/gen/common/net to be used by both the public and the viz APIs.

@alpeb
A couple more protobuf changes for the viz code split
0f0b616 
- Moved `healthcheck.proto` back from viz to `proto/common` as it remains being used by the main `healthcheck.go` library (it was moved to viz by #5510).
- Extracted from `viz.proto` the IP-related types and put them in `/controller/gen/common/net` to be used by both the public and the viz APIs.
@alpeb alpeb requested a review from a team as a code owner January 18, 2021 12:23
alpeb added a commit that referenced this pull request Jan 18, 2021
@alpeb
Chart templates for new viz linkerd-metrics-api pod
210c3f5 
This is based on the branch `alpeb/api-move` from #5553

The underlying go code for this will be pushed in a followup PR.
@alpeb alpeb mentioned this pull request Jan 18, 2021
Closed
Pothulapati
Pothulapati approved these changes Jan 18, 2021
View reviewed changes
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

LGTM, 👍 on moving the IP types to a common pkg

@alpeb alpeb changed the title A couple more protobuf changes for the viz code split public-api/viz split 1/8: a couple more protobuf changes Jan 18, 2021
@adleong
Copy link
Member

adleong commented Jan 19, 2021

As far as I can tell, the only place that healthcheck.proto is used in healthcheck.go is for the checkRPC machinery which only exists to call the SelfCheck API (which itself is moving to the viz API. I would expect that healthcheck.proto would move to the viz extension and all of the checkRPC stuff would be removed from healthcheck.go.

I think checkRPC was probably overly complicated to begin with and I think only existed so that a single call to SelfCheck could return multiple check results. Once this moves to the viz extension, I am perfectly okay simplifying this to be a regular check which returns a single result, just like everything else.

@alpeb
Copy link
Member Author

alpeb commented Jan 19, 2021 •
edited
Loading

@adleong that's a great call. In #5557 I have moved the SelfCheck API to viz. I'll see how easy it would be to make that change in #5557 and its downstream PRs, or as a separate effort altogether.

@alpeb
Copy link
Member Author

alpeb commented Jan 20, 2021

I figured the easiest would be to tackle the SelfCheck API refactoring separately, so I raised #5575 detailing the changes.

kleimkuhler
kleimkuhler reviewed Jan 20, 2021
View reviewed changes
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

SelfCheck refactoring aside, these changes look good.

@alpeb Just to make sure I'm following, you won't be making the SelfCheck refactoring changes as part of #5557 and will instead do this separately once this whole 1-8 merges?

@alpeb
Copy link
Member Author

alpeb commented Jan 20, 2021

@alpeb Just to make sure I'm following, you won't be making the SelfCheck refactoring changes as part of #5557 and will instead do this separately once this whole 1-8 merges?

Correct

kleimkuhler
kleimkuhler approved these changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

👍

@alpeb
Copy link
Member Author

alpeb commented Jan 20, 2021

Closing in favor of #5560 which will aggregate all these changes.

@alpeb alpeb closed this Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pothulapati Pothulapati Pothulapati approved these changes

@kleimkuhler kleimkuhler kleimkuhler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alpeb @adleong @kleimkuhler @Pothulapati