-
Notifications
You must be signed in to change notification settings - Fork 768
Use subpages for third party deps and API #8757
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.
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
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
I think our
What part you don't like and what would you like to improve? Maybe I can help you with that! |
cd "$PROJECT_DIR" | ||
go mod download | ||
go list -m -json all | "${TEMP_DIR}"/go-licence-detector \ | ||
-depsTemplate="${SCRIPT_DIR}"/templates/dependencies.md.tmpl \ | ||
-depsOut="${PROJECT_DIR}"/docs/reference/third-party-dependencies.md \ | ||
-template-value=eckVersion="${version}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relies on elastic/go-licence-detector#29
This PR is ready for review 🎉 , the preview is available here: https://docs-v3-preview.elastic.dev/elastic/cloud-on-k8s/pull/8757/reference |
@@ -1,2073 +1,13 @@ | |||
--- | |||
mapped_pages: | |||
- https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-api-reference.html | |||
navigation_title: API Reference | |||
navigation_title: API Reference (moved) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to preserve that page because it is referenced in the docs-content
repo, doc generation fails if I remove it. I believe we can fix that in a follow up PR once this one is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice work! As we release more versions of ECK we might have to revisit this strategy especially if there is an empty diff between versions it might make sense to think about grouping multiple versions in one document or something along those lines. But for now I am super happy we have a path forward.
@@ -4,7 +4,7 @@ mapped_pages: | |||
navigation_title: API Reference for 3.0.0 | |||
applies_to: | |||
deployment: | |||
eck: ga 3.0.0 | |||
eck: ga 3.0.0, ga 3.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pebrc I tested what we discussed out of band, here is the result:

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! This opens then indeed the door to collapse documentation for mulitple versions into one page.
This reverts commit 29d20d8.
@barkbay : should we remove the API reference and 3PP references docs identified as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just quickly diffed the two api references and I'm not seeing any substantive differences (one line about fleet advanced config). do we need to generate these as separate pages right now? do you expect substantive development to come to this area soon?
I also wonder how the versioning approach for the API packages (e.g. v1, v1beta1, v1alpha1) interact with the versioning for ECK itself. it feels possibly like the API reference needs to follow its own versioning scheme based on these hints.
if that's the case, maybe we could state which api versions are compatible with which stack versions as an alternative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one line about fleet advanced config
This is something new in ECK 3.1.0, which was not supported in 3.0.0. So I think I would keep 2 separate files.
I also wonder how the versioning approach for the API packages (e.g. v1, v1beta1, v1alpha1) interact with the versioning for ECK itself.
Each K8S API version stage can be used to represent a different maturity and stability level. When a new version is introduced it also requires a change in the operator. They are not related to stack versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something new in ECK 3.1.0, which was not supported in 3.0.0. So I think I would keep 2 separate files.
this is the substance of the change:


the change does not seem to a change to the API functionality (unless this is missing a related value for the new config model). Given this, I'd rather rely on the applicability information in the core docs than generate a whole new API reference for a clarifying sentence.
Each K8S API version stage can be used to represent a different maturity and stability level. When a new version is introduced it also requires a change in the operator. They are not related to stack versions.
it sounds like the version stage doesn't actually guarantee specific behavior independently of the eck version, so it still makes sense to use the ECK version as the primary versioning model 👍
if we still want to generate them for internal/dev use, we could consider making them hidden files so they can be visited but not consumed by readers who might be confused by them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall, this looks really great, and thank you for taking on the effort of refactoring the generation so quickly. my key piece of feedback is that we should think about the long-term vision for the API reference before we build a bunch of pages we need to maintain long term.
|
||
# Third-party dependencies [k8s-reference-dependencies] | ||
|
||
This section contains reference information about third-party dependencies used by {{eck}}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file should ideally link to the children in the section
@@ -1,6 +1,7 @@ | |||
toc: | |||
- file: index.md | |||
- file: api-docs.md | |||
- file: third-party-dependencies.md | |||
- folder: api-reference | |||
- folder: third-party-dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes 😕 , I think it would be a bit more complex to automate the generation of these files with the file/children model.
IIRC we have to document third-party dependencies for compliance purposes, not sure if this includes the ones currently referenced by the project in its |
Hiding the file for the |
Are hidden files compatible with the fact that we want to avoid the file/children model? |
unfortunately, I don't think so. if we needed to, we could ship this and revisit a more granular toc separately. |
This PR introduces some changes to generate third-party deps and API reference versioned pages.
Do not merge before 3.1