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

📖 docs updates for external types and submodule-layouts #3055

Merged
merged 2 commits into from
Jan 30, 2024
Merged

📖 docs updates for external types and submodule-layouts #3055

merged 2 commits into from
Jan 30, 2024

Conversation

jakobmoellerdev
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev commented Nov 2, 2022 •
edited
Loading

Fixes #2627 by adding sufficient documentation for external API reuse cases and multi/sub-module layouts.

From the proposed Overview:
This part describes how to modify a scaffolded project for use with multiple go.mod files for APIs and Controllers.

Sub-Module Layouts (in a way you could call them a special form of Monorepo's) are a special use case and can help in scenarios that involve reuse of APIs without introducing indirect dependencies that should not be available in the project consuming the API externally.

  • Write Introduction explaining what Sub-Module layout in kubebuilder is
  • Write Overview with Warnings from findings in RFE: Generate api sub-package, with (almost) no external dependencies. #2627 as well as explanation of use cases that it can solve
  • Write guide for v3 / v4alpha go projects for how to adjust the project
  • Move External Type usage to the book for easier visibility
  • Create Placeholder for External Type Documentation explaining that it got moved so that its easy to find for people who are used to the old location in docs instead of docs/reference
  • Refactor External Type usage for latest kubebuilder projects (minimum changes, after that decide on what else to change)
  • Reference External Type usage in Sub-Module Guide to reuse the API properly

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @jakobmoellersap. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2022
@jakobmoellerdev jakobmoellerdev marked this pull request as ready for review November 3, 2022 12:31
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2022
@everettraven
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2022
everettraven
everettraven reviewed Nov 8, 2022
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

@jakobmoellersap Thanks for the contribution! It looks good overall, I just had a couple comments/questions:

docs/book/src/reference/reference.md Show resolved Hide resolved
docs/book/src/reference/submodule-layouts.md Outdated Show resolved Hide resolved
docs/book/src/reference/using_an_external_type.md Outdated Show resolved Hide resolved
everettraven
everettraven approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2022
@jakobmoellerdev
Copy link
Contributor Author

/assign @varshaprasad96

camilamacedo86
camilamacedo86 reviewed Nov 22, 2022
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @jakobmoellersap,

That is a terrific work 🥇
Thank you. Could we just add a note like : https://github.com/kubernetes-sigs/kubebuilder/pull/3055/files#r1029177401 for we get this one merged?
WDYT?

@camilamacedo86
Copy link
Member

HI @everettraven

Maybe it makes sense to highlight that there are different ways to accomplish this rather than providing an explicit example and that it is up to the reader to implement what they feel is the best approach for their case?

All docs are based on examples/tutorials.
We need to keep this standard.

If you follow the steps that works, you can change things up.
Otherwise, makes it hard for a user to achieve the goal.

everettraven
everettraven approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2023
camilamacedo86
camilamacedo86 reviewed Dec 5, 2023
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @jakobmoellerdev

Really thank you for your amazing
I added the commits in the nits
Just replace domain for path
Just clarify that is not required to get done for coretypes
Also, clarify that the registration is not required for coretypes

And then, I am more than happy with we get this one merged
We can improve, etc in follow ups.

I know that was a hard journey but this content will help a lot the whole community
So, well done 🥇

@jakobmoellerdev @seh
@camilamacedo86 @everettraven
📖 refactor external type docs to be up to date
f208114 
Co-authored-by: Steven E. Harris <seh@panix.com>
Co-authored-by: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com>
Co-authored-by: Bryce Palmer <bpalmer@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2024
@jakobmoellerdev
Copy link
Contributor Author

After another hiatus for ages due to time, I finally implemented the PR comments. Sorry it took so long...

@camilamacedo86
Copy link
Member

Hi @jakobmoellerdev

After another hiatus for ages due to time, I finally implemented the PR comments. Sorry it took so long...

Your contribution has been great !
This PR is very big, so that is expected.
Thank you a lot

camilamacedo86
camilamacedo86 approved these changes Jan 30, 2024
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It was approved before @everettraven already
Now, the comments were addressed

I think it is shaped enough for us to move forward here
and we can work on improvements in follow-ups.

This PR is big and is a nice contribution 🥇
So, I think it is very valid we move forward and shape after (where/what might still be required.)

Therefore, I am

/lgtm
/approved

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, everettraven, jakobmoellerdev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2024
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Jan 30, 2024
@k8s-ci-robot k8s-ci-robot merged commit ba4754d into kubernetes-sigs:master Jan 30, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seh seh seh left review comments

@davidxia davidxia davidxia left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@jmrodri jmrodri Awaiting requested review from jmrodri

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

@everettraven everettraven Awaiting requested review from everettraven

Assignees

@camilamacedo86 camilamacedo86

@everettraven everettraven

@ArkaSaha30 ArkaSaha30

@varshaprasad96 varshaprasad96

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFE: Generate api sub-package, with (almost) no external dependencies.
9 participants
@jakobmoellerdev @k8s-ci-robot @everettraven @camilamacedo86 @ArkaSaha30 @Sajiyah-Salat @seh @davidxia @varshaprasad96