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

Add link to generated CRD doc #458

Merged
merged 3 commits into from
Feb 8, 2021
Merged

Add link to generated CRD doc #458

merged 3 commits into from
Feb 8, 2021

Conversation

upodroid
Copy link
Contributor

@upodroid upodroid commented Feb 4, 2021

Fixes: #432

@ravindk89
Copy link
Contributor

@harshavardhana @dvaldivia there was a discussion a while back about reducing the size of the CRD by removing some descriptor fields. Is that still a concern, where the generated docs might be inaccurate (or partial)?

@harshavardhana
Copy link
Member

@harshavardhana @dvaldivia there was a discussion a while back about reducing the size of the CRD by removing some descriptor fields. Is that still a concern, where the generated docs might be inaccurate (or partial)?

There doesn't seem to be the case, I have generated with the CRDs in master @ravindk89 and they seem to generate fine.

@dvaldivia
Copy link
Collaborator

@harshavardhana yes it's still a concern, the kubernetes API will reject the CRD if it's too large which happens with the two versions in a single CRD

@harshavardhana
Copy link
Member

@harshavardhana yes it's still a concern, the kubernetes API will reject the CRD if it's too large which happens with the two versions in a single CRD

does it still happen? @dvaldivia

@dvaldivia
Copy link
Collaborator

@harshavardhana yes if we add the descriptions

@harshavardhana
Copy link
Member

@harshavardhana yes if we add the descriptions

the crd.adoc is generated with whatever crd is in master - I am assuming this issue is handled in master @dvaldivia

@ravindk89
Copy link
Contributor

So based on @dvaldivia 's post, my assumption here is that eventually the CRD generation will not include the description fields, because we intend to (or already have) removed them.

We might still be able to use the CRD generation as just high-level fields. I can parse the adoc and compare it to what I've already documented so I can at least track changes to the CRD as they come. That way we can try to mix a best-of-two-worlds approach, with programmatic generation of the skeleton, but comprehensive reference docs that don't bog down the CRD size.

Thoughts @harshavardhana @dvaldivia

@harshavardhana
Copy link
Member

So based on @dvaldivia 's post, my assumption here is that eventually the CRD generation will not include the description fields, because we intend to (or already have) removed them.

We might still be able to use the CRD generation as just high-level fields. I can parse the adoc and compare it to what I've already documented so I can at least track changes to the CRD as they come. That way we can try to mix a best-of-two-worlds approach, with programmatic generation of the skeleton, but comprehensive reference docs that don't bog down the CRD size.

Thoughts @harshavardhana @dvaldivia

if we haven't removed them - can we remove them?

@ravindk89
Copy link
Contributor

@harshavardhana https://github.com/minio/operator/blob/master/resources/base/crds/minio.min.io_tenants.yaml the desc fields are already removed in master from what i can see

@harshavardhana
Copy link
Member

@harshavardhana https://github.com/minio/operator/blob/master/resources/base/crds/minio.min.io_tenants.yaml the desc fields are already removed in master from what i can see

Then generating doc is fine @ravindk89 its automatically able to add generic descriptions

@nitisht
Copy link
Contributor

nitisht commented Feb 8, 2021

Can we take this @ravindk89 @harshavardhana @dvaldivia

@ravindk89
Copy link
Contributor

Yeah @nitisht I think we're OK. My big action item is to migrate the independent work I've done on documenting these fields into the code base so the autogenerated CRD has more useful details. This will take time, but in the short term linking to the generated docs is still our route forward.

@harshavardhana harshavardhana merged commit a26e41e into minio:master Feb 8, 2021
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.

KES with Vault and Kubernetes Auth is broken.
5 participants