Skip to content

rocketmq: fix protobuf import path#17810

Closed
johanbrandhorst wants to merge 1 commit intoenvoyproxy:mainfrom
johanbrandhorst:patch-1
Closed

rocketmq: fix protobuf import path#17810
johanbrandhorst wants to merge 1 commit intoenvoyproxy:mainfrom
johanbrandhorst:patch-1

Conversation

@johanbrandhorst
Copy link
Copy Markdown

#17796 moved rocketmq to contrib, but forgot to update the relative import in v4alpha/rocketmq_proxy.proto as it did for v3/rocketmq_proxy.proto. Update the import to fix the Protobuf generation.

This was discovered when trying to build latest envoy with buf. Would you be interested
in adding some buf steps to your CI to avoid this sort of issue in the future?

Commit Message: rocketmq: fix protobuf import path
Additional Description: #17796 moved rocketmq to contrib, but forgot to update the relative import in v4alpha/rocketmq_proxy.proto as it did for v3/rocketmq_proxy.proto. Update the import to fix the Protobuf generation.
Risk Level: Low
Testing: Existing tests
Docs Changes: None
Release Notes: None

@repokitteh-read-only
Copy link
Copy Markdown

Hi @johanbrandhorst, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17810 was opened by johanbrandhorst.

see: more, trace.

envoyproxy#17796 moved rocketmq to `contrib`, but
forgot to update the relative import in v4alpha/rocketmq_proxy.proto as it did
for v3/rocketmq_proxy.proto. Update the import to fix the Protobuf generation.

This was discovered when trying to build latest envoy with `buf`. Would you be interested
in adding some `buf` steps to your CI to avoid this sort of issue in the future?

Signed-off-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
@johanbrandhorst
Copy link
Copy Markdown
Author

CC @mattklein123

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks. I think the issue is we don't actually build v4alpha right now and these are all going to get deleted soon I think so probably not worth it to add anything to CI. Thanks for fixing.

@mattklein123
Copy link
Copy Markdown
Member

Hmm, looks like format is failing probably due to the proto format code not working quite right in v4alpha generation. @htuch is this worth fixing since it's not failing our CI and we are going to remove soon?

/wait

@htuch
Copy link
Copy Markdown
Member

htuch commented Aug 23, 2021

@mattklein123 yeah, v4alpha is not long for this world in the repo, I'll get around to removing after #17724 lands.

@YaseenAlk
Copy link
Copy Markdown
Contributor

@johanbrandhorst I've been working on adding buf to CI actually! #17793

It actually caught this same issue, you beat me to it: https://dev.azure.com/cncf/envoy/_build/results?buildId=86148&view=logs&j=b4781dd6-e8d1-56ca-824c-e97e27bdf79d&t=45617383-04d6-5506-f07f-2ba46008d9c7&l=30168

Also, the pre-push hooks seemed to have some trouble when I tried to make this same change on my own fork.

@johanbrandhorst
Copy link
Copy Markdown
Author

Looks like this was fixed on main, I'll close this PR.

@johanbrandhorst johanbrandhorst deleted the patch-1 branch August 27, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants