Skip to content

Add protoc-gen-gotag plugin to builder image #28943

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
Closed
wants to merge 1 commit into from
Closed

Add protoc-gen-gotag plugin to builder image #28943

wants to merge 1 commit into from

Conversation

marqc
Copy link
Contributor

@marqc marqc commented Nov 2, 2023

Please ensure your pull request adheres to the following guidelines:

  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.

Adds a protoc-gen-gotag plugin to generate yaml annotations in the generated proto as discussed in #28873 (comment)

cc: @chancez @glibsm

Adds a protoc-gen-gotag plugin

@marqc marqc requested a review from a team as a code owner November 2, 2023 12:10
@marqc marqc requested a review from rolinh November 2, 2023 12:10
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 2, 2023
@marqc marqc had a problem deploying to release-base-images November 2, 2023 12:10 — with GitHub Actions Error
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 2, 2023
This was referenced Nov 2, 2023
Closed
Merged
@marqc marqc force-pushed the protoc_gotag_plugin branch from 82e136e to 4f74bf8 Compare November 2, 2023 12:46
@marqc marqc requested review from a team as code owners November 2, 2023 12:46
@marqc marqc requested review from christarazi and bimmlerd November 2, 2023 12:46
@marqc marqc had a problem deploying to release-base-images November 2, 2023 12:47 — with GitHub Actions Error
@marqc marqc force-pushed the protoc_gotag_plugin branch from 4f74bf8 to ce6642f Compare November 2, 2023 12:57
@marqc marqc had a problem deploying to release-base-images November 2, 2023 12:57 — with GitHub Actions Failure
@rolinh
Copy link
Member

rolinh commented Nov 2, 2023

I think we need to build a new builder image for the change to effectively take effect, cf https://docs.cilium.io/en/latest/contributing/development/images/#update-cilium-builder-and-cilium-runtime-images
Feel free to ping me to approve the deployments.

@marqc marqc force-pushed the protoc_gotag_plugin branch from ce6642f to c51f278 Compare November 2, 2023 17:50
@marqc marqc temporarily deployed to release-base-images November 2, 2023 17:50 — with GitHub Actions Inactive
@marqc marqc force-pushed the protoc_gotag_plugin branch from c51f278 to 59abe5b Compare November 2, 2023 17:55
@marqc marqc temporarily deployed to release-base-images November 2, 2023 17:55 — with GitHub Actions Inactive
@christarazi
Copy link
Member

I see that the commit in #28944 is the same as the commit here? Is this intended?

@marqc marqc force-pushed the protoc_gotag_plugin branch from 59abe5b to cb6c842 Compare November 2, 2023 21:23
@marqc marqc temporarily deployed to release-base-images November 2, 2023 21:23 — with GitHub Actions Inactive
@marqc
Copy link
Contributor Author

marqc commented Nov 2, 2023

I see that the commit in #28944 is the same as the commit here? Is this intended?

@christarazi I'm not sure if changing the build image can be submitted together with changes using it. That's why this commit is included in both PRs. Once the other PR gets merged this one will be obsolete. If this PR is merged first, the other one will only contain changes after using this build image.

@christarazi
Copy link
Member

I'm not sure if changing the build image can be submitted together with changes using it.

I am not aware of any problems with the build image being submitted together. I think we should probably close this PR to avoid confusion.

@christarazi
Copy link
Member

Oh I see Chance gave details on this and I don't want to be giving conflicting feedback, so please ignore my message above.

@rolinh rolinh added the release-note/misc This PR makes changes that have no direct user impact. label Nov 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 3, 2023
@rolinh
Copy link
Member

rolinh commented Nov 3, 2023

/test

bimmlerd
bimmlerd approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

fine by me, but will similarly conflict with #28945; let's just see who gets in first.

@chancez
Copy link
Contributor

chancez commented Nov 3, 2023

Oh I see Chance gave details on this and I don't want to be giving conflicting feedback, so please ignore my message above.

I also had no idea we could do a single PR that modified the base image 😮

@marqc marqc force-pushed the protoc_gotag_plugin branch from cb6c842 to 8334f42 Compare November 6, 2023 21:31
@marqc marqc had a problem deploying to release-base-images November 6, 2023 21:31 — with GitHub Actions Failure
@marqc marqc force-pushed the protoc_gotag_plugin branch from 8334f42 to 8cef10b Compare November 7, 2023 08:20
@marqc marqc temporarily deployed to release-base-images November 7, 2023 08:20 — with GitHub Actions Inactive
@marqc
Copy link
Contributor Author

marqc commented Nov 7, 2023

fine by me, but will similarly conflict with #28945; let's just see who gets in first.

Rebased

@rolinh
Copy link
Member

rolinh commented Nov 7, 2023

/test

@marqc marqc force-pushed the protoc_gotag_plugin branch from 8cef10b to 1603f99 Compare November 7, 2023 14:14
@marqc marqc temporarily deployed to release-base-images November 7, 2023 14:14 — with GitHub Actions Inactive
@rolinh
Copy link
Member

rolinh commented Nov 7, 2023

/test

@marqc marqc force-pushed the protoc_gotag_plugin branch from 1603f99 to 6fddb88 Compare November 7, 2023 22:28
@marqc marqc temporarily deployed to release-base-images November 7, 2023 22:28 — with GitHub Actions Inactive
@julianwiedmann julianwiedmann added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Nov 14, 2023
@marqc marqc force-pushed the protoc_gotag_plugin branch from 6fddb88 to 2330177 Compare November 15, 2023 10:08
@marqc marqc had a problem deploying to release-base-images November 15, 2023 10:08 — with GitHub Actions Failure
@marqc
Copy link
Contributor Author

marqc commented Nov 15, 2023

@julianwiedmann rebased, can you trigger the github action to build the image, so I can update definitions

@rolinh
Copy link
Member

rolinh commented Nov 15, 2023

can you trigger the github action to build the image

Just did it.

@marqc
Add protoc-gen-gotag plugin to builder image.
7215fa2 
Plugin will be used to generate yaml annotation in structs generated
from proto to be able to unmarshall FlowFilter from YAML for cilium#25508.

Signed-off-by: Marek Chodor <mchodor@google.com>
@marqc marqc force-pushed the protoc_gotag_plugin branch from 2330177 to 7215fa2 Compare November 15, 2023 11:28
@marqc marqc temporarily deployed to release-base-images November 15, 2023 11:28 — with GitHub Actions Inactive
@marqc
Copy link
Contributor Author

marqc commented Nov 15, 2023

can you trigger the github action to build the image

Just did it.

thanks, image references updated

@rolinh
Copy link
Member

rolinh commented Nov 15, 2023

/test

@marqc
Copy link
Contributor Author

marqc commented Nov 16, 2023

I just realized that names generated with this plugin are inconsitent with json tags. This would make a mess in the API. I will close this and instead I will use different YAML parser that works with json tags sigs.k8s.io/yaml

@marqc marqc closed this Nov 16, 2023
skmatti pushed a commit to skmatti/cilium that referenced this pull request Jul 24, 2024
@marqc @yasz24
Introduce dynamic hubble flow logs exporters.
89626df 
Add protoc-gen-gotag plugin to builder image.
Add yaml annotations to flow proto gen files.
Introduce dynamic hubble flow logs exporters based on config file.

Signed-off-by: Marek Chodor <mchodor@google.com>
Carry-Patch-Approval: b/309936968
Upstream-PR: cilium#28873
Upstream-PR: cilium#28943
Upstream-PR: cilium#28944
Change-Id: I7871d8b119822c3228a6e959b86dd1b6b23d275a
Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/860928
Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Reviewed-by: Akhil Velagapudi <avelagap@google.com>
Reviewed-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Tested-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@chancez chancez chancez approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@christarazi christarazi Awaiting requested review from christarazi christarazi is a code owner automatically assigned from cilium/ci-structure

Assignees
No one assigned
Labels
dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@marqc @rolinh @christarazi @chancez @bimmlerd @julianwiedmann