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.

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

Remove outdated helm values docs #117

Merged
merged 3 commits into from
Nov 1, 2024

Conversation

killianmuldoon
Copy link
Contributor

@killianmuldoon killianmuldoon commented Oct 21, 2024

Related PR on the Network Operator repo: Mellanox/network-operator#1121

Signed-off-by: killianmuldoon <[email protected]>
@killianmuldoon
Copy link
Contributor Author

It looks like we're losing quite a bit of information from these changes - but a lot of it is duplicated in the other configuration guides so I don't believe we're losing anything unique, just the helm configuration flow.

@rollandf rollandf requested a review from e0ne October 21, 2024 09:51
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
docs/customizations/helm.rst Show resolved Hide resolved
.. warning::
The InfiniBand Fabric manages a single pool of GUIDs. In order to use IB Kubernetes in different clusters, different GUID ranges must be specified to avoid collisions.

==================
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels like we should have a section explaining each component. previously it was done under helm docs.
(which was not ideal i agree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT about doing that in the godoc and having it be part of the API docs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont think this should be a general rule. only when it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added basic info and links to each component in the godoc - to me it makes sense and is useful, but let's discuss it on the Network Operator MR

- `"v0.2.0"`
- NVIDIA IPAM Plugin image version.

.. warning::
Copy link
Collaborator

Choose a reason for hiding this comment

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

this bit of information is important for nvidia-k8s-ipam

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we add it in the godoc instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this one should remain in rst only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a version of this in the godoc - but let's discuss it on the other PR and then come back to this one.

@adrianchiris
Copy link
Collaborator

thx for the cleanup @killianmuldoon ! added a couple of comments in addition to freddy.

@killianmuldoon
Copy link
Contributor Author

I've added the missing information in the goods of the API where feasible in this PR: Mellanox/network-operator#1121

docs/advanced-configurations.rst Outdated Show resolved Hide resolved
docs/advanced-configurations.rst Show resolved Hide resolved
docs/advanced-configurations.rst Show resolved Hide resolved
Signed-off-by: killianmuldoon <[email protected]>
@rollandf rollandf merged commit 90da05e into Mellanox:main Nov 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants