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

CODEOWNERS: create new group hubble-metrics #35991

Merged
merged 1 commit into from
Dec 11, 2024
Merged

CODEOWNERS: create new group hubble-metrics #35991

merged 1 commit into from
Dec 11, 2024
+11 −5

Conversation

rectified95
Copy link
Contributor

Adding new group for the subset of Hubble code dealing with metrics generation, handler registration etc.

@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 14, 2024
@rectified95 rectified95 force-pushed the make_sig-hubble-metrics branch from 4a7b296 to d551848 Compare November 14, 2024 19:16
@rectified95 rectified95 changed the title CODEOWNERS: create new group sig-hubble-metrics CODEOWNERS: create new group hubble-metrics Nov 14, 2024
@joestringer joestringer requested review from a team and rolinh and removed request for a team November 20, 2024 00:42
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Nov 20, 2024
@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 20, 2024
joestringer
joestringer reviewed Nov 21, 2024
View reviewed changes
@rectified95 rectified95 closed this Dec 3, 2024
@rectified95 rectified95 force-pushed the make_sig-hubble-metrics branch from 5419e98 to 8da6f36 Compare December 3, 2024 19:59
@rectified95 rectified95 reopened this Dec 3, 2024
@rectified95 rectified95 marked this pull request as ready for review December 3, 2024 20:02
@rectified95 rectified95 requested a review from a team as a code owner December 3, 2024 20:02
@rectified95 rectified95 requested a review from bimmlerd December 3, 2024 20:02
@rectified95
Copy link
Contributor Author

rectified95 commented Dec 3, 2024 •
edited
Loading

@joestringer Does someone with write permissions need to create the new team and parent it to "Cilium Teams"?
https://github.com/orgs/cilium/new-team?parent_team=cilium-teams

@joestringer
Copy link
Member

@rectified95 sure, let's get cilium/community#172 approved and merged and I'll ensure it gets reflected into the backend. I want to make sure that the team is created and available before we merge this PR delegating control to the new group, just in case any in-flight PRs are impacted by the changes.

@joestringer joestringer added the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Dec 3, 2024
@joestringer
Copy link
Member

Also looks like the docs need an update. make -C Documentation update-codeowners should do the trick.

rolinh
rolinh requested changes Dec 4, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Actually, I think we want to remove sig-hubble-api from codeowners to the hubble metrics subsystem, see rationale below.

@rectified95 rectified95 force-pushed the make_sig-hubble-metrics branch from a6b34dc to d9fca51 Compare December 4, 2024 20:01
@rectified95 rectified95 requested a review from a team as a code owner December 4, 2024 20:01
@rectified95 rectified95 requested a review from qmonnet December 4, 2024 20:01
rolinh
rolinh requested changes Dec 4, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Could we please also remove the reference to Hubble metrics from the description of the responsibilities of the sig-hubble-api group given the change in scope?

joestringer
joestringer approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

The new group is created and populated. Once you've addressed Robin's feedback this should be good to merge.

@joestringer joestringer removed the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Dec 9, 2024
@qmonnet qmonnet removed the request for review from bimmlerd December 9, 2024 17:20
@qmonnet qmonnet removed their request for review December 9, 2024 17:20
@aanm aanm requested a review from rolinh December 10, 2024 12:27
@aanm aanm enabled auto-merge December 10, 2024 12:27
@aanm
Copy link
Member

aanm commented Dec 10, 2024

/test

@rolinh rolinh disabled auto-merge December 10, 2024 15:30
@rolinh
Copy link
Member

rolinh commented Dec 10, 2024

@aanm Sorry to disable auto-merge but my feedback has not been addressed.

rolinh
rolinh requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Re-requesting a change as my feedback was dismissed

Could we please also remove the reference to Hubble metrics from the description of the responsibilities of the sig-hubble-api group given the change in scope?

@rectified95 rectified95 force-pushed the make_sig-hubble-metrics branch from d9fca51 to e829413 Compare December 10, 2024 21:10
@rectified95
Copy link
Contributor Author

@rolinh Thanks for bubbling this up for me, comment addressed .

@rectified95
create new group cilium/hubble-metrics
Loading
Loading status checks…
3da2e83 
Signed-off-by: Igor Klemenski <igor.klemenski@microsoft.com>
@rectified95 rectified95 force-pushed the make_sig-hubble-metrics branch from e829413 to 3da2e83 Compare December 10, 2024 21:21
@joestringer joestringer requested a review from rolinh December 10, 2024 21:35
rolinh
rolinh approved these changes Dec 11, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

@rectified95 Thank you, lgtm!

@rolinh rolinh enabled auto-merge December 11, 2024 08:13
@rolinh
Copy link
Member

rolinh commented Dec 11, 2024

/test

@rolinh rolinh added this pull request to the merge queue Dec 11, 2024
Merged via the queue into cilium:main with commit e3aba63 Dec 11, 2024
66 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Dec 11, 2024
@cilium-release-bot cilium-release-bot bot mentioned this pull request Dec 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer

@rolinh rolinh

@chancez chancez

@michi-covalent michi-covalent

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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.

None yet
6 participants
@rectified95 @joestringer @aanm @rolinh @chancez @michi-covalent