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: IPsec owns pkg/common/ipsec #29002

Merged
merged 1 commit into from Nov 6, 2023
Merged

CODEOWNERS: IPsec owns pkg/common/ipsec #29002

merged 1 commit into from Nov 6, 2023

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Nov 6, 2023

This directory was introduced in commit 2218611 ("cmd, common: Move countUniqueIPsecKeys to common/ipsec pkg"), but the CODEOWNERS were not updated.

Fixes: 2218611 ("cmd, common: Move countUniqueIPsecKeys to common/ipsec pkg")

@pchaigno
CODEOWNERS: IPsec owns pkg/common/ipsec
13d172c 
This directory was introduced in commit 2218611 ("cmd, common:
Move countUniqueIPsecKeys to common/ipsec pkg"), but the CODEOWNERS were
not updated.

Fixes: 2218611 ("cmd, common: Move countUniqueIPsecKeys to common/ipsec pkg")
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno pchaigno added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/misc This PR makes changes that have no direct user impact. sig/contributing Impacts contribution workflow, guidelines, and tools. labels Nov 6, 2023
@pchaigno pchaigno requested a review from a team as a code owner November 6, 2023 11:30
bimmlerd
bimmlerd approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment •
edited

Choose a reason for hiding this comment

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

Changes are fine, but I'll just note that https://google.github.io/styleguide/go/best-practices.html#util-packages common is a bit of a red-flag in terms of package naming.

EDIT: or to be more clear, common/ipsec is specifically fine just not happy we have a common pkg with logic in it)

@pchaigno
Copy link
Member Author

pchaigno commented Nov 6, 2023

Changes are fine, but I'll just note that https://google.github.io/styleguide/go/best-practices.html#util-packages common is a bit of a red-flag in terms of package naming.

EDIT: or to be more clear, common/ipsec is specifically fine just not happy we have a common pkg with logic in it)

Feel free to rename. I don't have a dog in this fight.

@bimmlerd
Copy link
Member

bimmlerd commented Nov 6, 2023

Changes are fine, but I'll just note that https://google.github.io/styleguide/go/best-practices.html#util-packages common is a bit of a red-flag in terms of package naming.
EDIT: or to be more clear, common/ipsec is specifically fine just not happy we have a common pkg with logic in it)

Feel free to rename. I don't have a dog in this fight.

Yep, it's now on the list 😇

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 6, 2023
@pchaigno pchaigno merged commit 4e8df52 into cilium:main Nov 6, 2023
60 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bimmlerd bimmlerd bimmlerd approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. 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. sig/contributing Impacts contribution workflow, guidelines, and tools.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pchaigno @bimmlerd