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

chore: Regenerate protobuf code #375

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ctlong
Copy link
Member

@ctlong ctlong commented Sep 1, 2023

No description provided.

Prepending `code.cloudfoundry.org` generates the code automatically into
`code.cloudfoundry.org/...`, which is not the path that we store the
loggregator-agent-release repo at typically. If we remove it, the
protobuf code generation step becomes a lot simpler.

Besides, we weren't even adding that line to our package name in the
generated files, so it's not very helpful. Better to have the path align
with where we want the generated files to actually go.
@acrmp
Copy link
Member

acrmp commented Sep 6, 2023

@ctlong It looks like the doppler.proto here is really old:

message ContainerMetricsRequest {
string appID = 1;
}
message ContainerMetricsResponse {
repeated bytes payload = 1;
}
message RecentLogsRequest {
string appID = 1;
}
message RecentLogsResponse {
repeated bytes payload = 1;
}

What do you think about the current pattern where doppler.proto is duplicated in loggregator-release and here?

@chombium
Copy link
Contributor

chombium commented Sep 8, 2023

@ctlong, @acrmp I don't know about the ContainerMetrics request, but I'm not sure if we can remove the RecentLogs as we keep one older v106.7.x for the loggregator-release in maintenance mode where the endpoint is still there in the Traffic Controller.

If we decide to remove them, we should do a proper deprecation process, so that everyone is informed, and make sure that the other protocol definitions are compatible with the older releases as one will have to pin the loggregator-release version.

@ctlong
Copy link
Member Author

ctlong commented Sep 12, 2023

Good point @chombium. Maybe this PR is okay as it is then. What do you think @acrmp?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting for Changes | Open for Contribution
Development

Successfully merging this pull request may close these issues.

None yet

3 participants