Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

⚠️ Migrate information away from docs/api.md #1820

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

dtantsur
Copy link
Member

@dtantsur dtantsur commented Jul 8, 2024

The motivation of this change is to avoid duplicating information
between the inline API docs, the user guide and this page.

Some useful bits of information have been migrated to the inline docs,
which I also proof-read for mistakes. Then most of the content of api.md
has been replaced with references to the rendered CR docs and the user
guide.

Signed-off-by: Dmitry Tantsur dtantsur@protonmail.com

The motivation of this change is to avoid duplicating information
between the inline API docs, the user guide and this page.

Some useful bits of information have been migrated to the inline docs,
which I also proof-read for mistakes. Then most of the content of api.md
has been replaced with references to the rendered CR docs and the user
guide.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
@metal3-io-bot metal3-io-bot requested review from Rozzii and zaneb July 8, 2024 12:29
@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 8, 2024
@Rozzii
Copy link
Member

Rozzii commented Jul 9, 2024

I would like you to please mention the apis/metal3.io/v1alpha1/baremetalhost_types.go in the new api.md just to make it more easily discoverable, there are info now in that file that might help someone better understand the BMO's features.
Otherwise LGTM.

Copy link
Member

@mboukhalfa mboukhalfa left a comment

Choose a reason for hiding this comment

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

Remember someone might come and argue that inline comments are not documentation!

Although godoc can be easily updated with the code changes not sure if kubebuilder has any best practices for documenting the API some might want docs without going through the code I won't expect the user-guide to have a detailed docs for an API even we cannot force it be up to date with the API changes like being in the same repo

@dtantsur
Copy link
Member Author

dtantsur commented Jul 9, 2024

I would like you to please mention the apis/metal3.io/v1alpha1/baremetalhost_types.go in the new api.md just to make it more easily discoverable

Aren't the already provided links to the rendered CRD docs better? Especially given that

inline comments are not documentation!

@mboukhalfa
Copy link
Member

mboukhalfa commented Jul 9, 2024

Aren't the already provided links to the rendered CRD docs better?

We can discuss that if we assume the user has bmo running before checking the API docs

@dtantsur
Copy link
Member Author

dtantsur commented Jul 9, 2024

We can discuss that if we assume the user has bmo running before checking the API docs

I'm not sure what you're talking about. Could you please double-check the changes to docs/api.md here? I'm adding links to https://doc.crds.dev/github.com/metal3-io/baremetal-operator/, is it something you don't consider acceptable?

In any case, having the same information in 2 places is a recipe for getting outdated information.

@mboukhalfa
Copy link
Member

mboukhalfa commented Jul 9, 2024

https://doc.crds.dev/github.com/metal3-io/baremetal-operator/

That a good idea I was thinking CRDs docs rendered with kubectl

@dtantsur dtantsur requested a review from mboukhalfa July 10, 2024 09:03
@dtantsur
Copy link
Member Author

@Rozzii @mboukhalfa could you re-review given what we've discussed? I believe your requests are already addressed in a slightly better way.

@mboukhalfa
Copy link
Member

mboukhalfa commented Jul 10, 2024

@Rozzii @mboukhalfa could you re-review given what we've discussed? I believe your requests are already addressed in a slightly better way.

I will check a bit later in details and might give some feedback I think I do not have the right to lgtm on this repo, I was wondering where the examples like HostFirmwareSettings Example got migrated to ?

@dtantsur
Copy link
Member Author

where the examples like HostFirmwareSettings Example got migrated to ?

https://book.metal3.io/bmo/firmware_settings

@Rozzii
Copy link
Member

Rozzii commented Jul 10, 2024

I would like you to please mention the apis/metal3.io/v1alpha1/baremetalhost_types.go in the new api.md just to make it more easily discoverable

Aren't the already provided links to the rendered CRD docs better? Especially given that

inline comments are not documentation!

I meant mentioning the apis/metal3.io/v1alpha1/baremetalhost_types.go in addition to the auto generated CRD docs and the book. I know it overlaps with the CRDs but IMO it still have info that is not present on the auto generated CRDs e.g. https://github.com/metal3-io/baremetal-operator/pull/1820/files#diff-0f3e356f6156566888fdbc0e7c0fa713ee054933106059fc29f702f8d1a554f2R365-R426

Like the software and hardware raid are mutually exclusive or that the hostFirmwareSetting CR is an alternative to the BMH firmware field. Just tiny bits of info that might help someone connect the dots better.

My request is just to simply put a line in the api.md that say something like this "Additional API implementation details can be found in apis/metal3.io/v1alpha1/baremetalhost_types.go " , just to let the newcomers know that we hove some info bits there too.

@dtantsur
Copy link
Member Author

Like the software and hardware raid are mutually exclusive or that the hostFirmwareSetting CR is an alternative to the BMH firmware field. Just tiny bits of info that might help someone connect the dots better.

This is a case for a user guide IMO.

My request is just to simply put a line in the api.md that say something like this "Additional API implementation details can be found in apis/metal3.io/v1alpha1/baremetalhost_types.go "

Will do

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2024
Copy link
Member

@mboukhalfa mboukhalfa left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest, mboukhalfa

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2024
@metal3-io-bot metal3-io-bot merged commit c5a77d2 into metal3-io:main Sep 4, 2024
19 checks passed
// If unspecified or set be 0, the maximum capacity of disk will be used for logical disk.
// +kubebuilder:validation:Minimum=0
SizeGibibytes *int `json:"sizeGibibytes,omitempty"`

// RAID level for the logical disk. The following levels are supported: 0;1;1+0.
// RAID level for the logical disk. The following levels are supported:
// 0, 1 and 1+0.
Copy link
Member

Choose a reason for hiding this comment

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

No Oxford comma? 🫢

Copy link
Member Author

Choose a reason for hiding this comment

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

You expect too much of my English grammar :)

BMC BMCDetails `json:"bmc,omitempty"`

// RAID configuration for bare metal server
// RAID configuration for bare metal server. If set, the RAID settings
// will be applied before the host is provisioned. If not, the current
Copy link
Member

Choose a reason for hiding this comment

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

to be more precise, before the host is "ready", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

"ready" is not a status, so it can be confusing

Copy link
Member

Choose a reason for hiding this comment

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

sorry, meant to say "available"

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: Done / Closed
Development

Successfully merging this pull request may close these issues.

6 participants