Skip to content

docs: Add documentation for cilium/vendor reponsibilities #34211

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
Merged
merged 1 commit into from
Nov 4, 2024
Merged

docs: Add documentation for cilium/vendor reponsibilities #34211

merged 1 commit into from
Nov 4, 2024

Conversation

learnitall
Copy link
Contributor

This commit adds a new page to the contributing section of Cilium's documentation to document the responsibilities of the cilium/vendor team, expanding on the description found in the CODEOWNERS file.

@learnitall learnitall added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Aug 6, 2024
@learnitall learnitall requested review from a team as code owners August 6, 2024 15:32
@learnitall learnitall requested review from tklauser and a user August 6, 2024 15:32
@learnitall learnitall force-pushed the pr/learnitall/vendor branch from 0665b76 to 4c8dca5 Compare August 6, 2024 15:51
joestringer
joestringer reviewed Aug 6, 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.

Some minor feedback, mostly formatting / maintainability. I would welcome more review particularly from @cilium/vendor who this affects.

@joestringer joestringer requested review from a team August 6, 2024 19:07
tklauser
tklauser reviewed Aug 7, 2024
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks for adding this. Left a few minor comments and a suggestion on expanding the section regarding replace directives. Happy to hear other @cilium/vendor member's opinion too.

rolinh
rolinh reviewed Aug 7, 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.

Thanks for documenting this Ryan! Although I'm no longer part of the vendor team, I gave it a good read. I think it captures the responsibilities of the vendor team well except for the fact that it doesn't mention the dep's license as being a criteria that must be met when a new dep is added (see comment below).

ovidiutirla
ovidiutirla reviewed Aug 13, 2024
View reviewed changes
Copy link
Contributor

@ovidiutirla ovidiutirla left a comment

Choose a reason for hiding this comment

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

Added a few comments.
Nice work 🚀 Thanks!

ovidiutirla
ovidiutirla reviewed Aug 13, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 13, 2024
@github-actions GitHub Actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Sep 27, 2024
@rolinh
Copy link
Member

rolinh commented Sep 27, 2024

@learnitall Shall we re-open? I think this doc is very valuable.

@learnitall
Copy link
Contributor Author

Yes definitely, I've been a tad short on cycles and lost sight of this. I'll get to the feedback as soon as I can.

@learnitall learnitall reopened this Sep 27, 2024
@tklauser tklauser removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 27, 2024
@learnitall learnitall force-pushed the pr/learnitall/vendor branch from 4c8dca5 to 34c796a Compare October 17, 2024 21:33
@learnitall
Copy link
Contributor Author

Thanks for the feedback everybody! My apologies for the delay. I just updated the PR based on y'alls feedback.

@learnitall learnitall force-pushed the pr/learnitall/vendor branch 2 times, most recently from 59dad6d to e5d6599 Compare October 18, 2024 15:10
ovidiutirla
ovidiutirla approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@ovidiutirla ovidiutirla left a comment

Choose a reason for hiding this comment

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

Looking better! Thanks!

ghost
ghost approved these changes Oct 21, 2024
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

one typo, but lgtm overall

joestringer
joestringer approved these changes Oct 24, 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.

Couple of minor wording suggestions to go with @lambdanis' review. With those fixed up I think this is good to go in.

We can continue to discuss the open threads, I don't want to shoot down the great discussion. I also want to make sure we don't make perfect the enemy of good and defer this useful guidance out for another few weeks. The text makes a lot of sense and we can always iterate further on it when it's in the tree.

@learnitall
docs: Add documentation for cilium/vendor reponsibilities
ac33515 
This commit adds a new page to the contributing section of Cilium's
documentation to document the responsibilities of the cilium/vendor
team, expanding on the description found in the CODEOWNERS file.

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
@learnitall learnitall force-pushed the pr/learnitall/vendor branch from e5d6599 to ac33515 Compare October 25, 2024 16:07
@joestringer
Copy link
Member

/test

@joestringer joestringer enabled auto-merge October 25, 2024 21:33
@aanm aanm requested review from aanm and removed request for network-charles October 28, 2024 15:57
@joestringer joestringer added this pull request to the merge queue Nov 4, 2024
Merged via the queue into cilium:main with commit 129e0f6 Nov 4, 2024
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xmulligan xmulligan xmulligan left review comments

@tklauser tklauser tklauser approved these changes

@joestringer joestringer joestringer approved these changes

@rolinh rolinh rolinh approved these changes

@ovidiutirla ovidiutirla ovidiutirla approved these changes

@aanm aanm Awaiting requested review from aanm

+2 more reviewers

@network-charles network-charles network-charles left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. 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.
7 participants
@learnitall @rolinh @joestringer @tklauser @ovidiutirla @xmulligan @network-charles