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

Fix truncated descriptions in the CRDs #2563

Merged
merged 12 commits into from
Apr 11, 2024

Conversation

iblancasa
Copy link
Contributor

Fixes #2561.

@iblancasa iblancasa changed the title Fix truncated descriptions in the documentation [chore] Fix truncated descriptions in the documentation Jan 24, 2024
@iblancasa iblancasa changed the title [chore] Fix truncated descriptions in the documentation Fix truncated descriptions in the documentation Jan 24, 2024
@iblancasa iblancasa changed the title Fix truncated descriptions in the documentation Fix truncated descriptions in the CRDs Jan 24, 2024
Signed-off-by: Israel Blancas <[email protected]>
@iblancasa iblancasa marked this pull request as ready for review January 24, 2024 22:09
@iblancasa iblancasa requested a review from a team January 24, 2024 22:09
@jaronoff97
Copy link
Contributor

@iblancasa we had to do this because we were reaching a limit for the size of a crd's annotations field. I'm not sure there's a way around this... cc @pavolloffay

@iblancasa
Copy link
Contributor Author

Ok, I see. Since the problem is with the generated docs... how about increasing the limit just for the docs? I can generate the CRDs without a limit in the description and generate the documentation from there. The CRDs shipped as part of the bundle will remain intact.

Also, @swiatekm-sumo suggestion sounds like something we should do too.

What do you think?

@pavolloffay
Copy link
Member

The max description length was decreased in #1981. I would prefer a solution where we could decide which fields would be truncated.

@iblancasa
Copy link
Contributor Author

The max description length was decreased in #1981. I would prefer a solution where we could decide which fields would be truncated.

I'm not sure that can be done except doing them shorter.

Makefile Outdated
@@ -479,8 +480,11 @@ bundle-push:
api-docs: crdoc kustomize
@{ \
set -e ;\
TMP_MANIFEST_DIR=$$(mktemp -d) ; \
cp -r config/crd/* $$TMP_MANIFEST_DIR; \
$(MAKE) CRD_OPTIONS=$(CRD_OPTIONS),maxDescLen=800 MANIFEST_DIR=$$TMP_MANIFEST_DIR/bases manifests ;\
Copy link
Member

Choose a reason for hiding this comment

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

Why here is ,maxDescLen=800 and the length is also defined in CRD_OPTIONS ?= "crd:generateEmbeddedObjectMeta=true,maxDescLen=200" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will call the manifests target using CRD_OPTIONS="crd:generateEmbeddedObjectMeta=true,maxDescLen=200",maxDescLen=800. The second maxDescLen overrides the first one. By doing this, we can:

  • Reuse the value of CRD_OPTIONS
  • Use the new value for maxDescLen without polluting the bundle

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need the max length for the api-docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is easier to add a high value than implementing a sed command to remove it from the command 😅

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

@jaronoff97 @pavolloffay we need to merge this if we want to actually release v1beta1.

@jaronoff97
Copy link
Contributor

i'm good to merge this, @pavolloffay any objections?

@pavolloffay
Copy link
Member

@iblancasa could you please rebase this one?

@swiatekm swiatekm requested a review from pavolloffay April 8, 2024 10:28
@pavolloffay pavolloffay merged commit 17abca5 into open-telemetry:main Apr 11, 2024
31 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Fix truncated descriptions in the documentation

Signed-off-by: Israel Blancas <[email protected]>

* Add other generated files

Signed-off-by: Israel Blancas <[email protected]>

* Undo change in version

Signed-off-by: Israel Blancas <[email protected]>

* Add changelog

Signed-off-by: Israel Blancas <[email protected]>

* Revert changes in manifest CRDs. Add a way to generate the long descriptions in the MD

Signed-off-by: Israel Blancas <[email protected]>

* Fix changelog

Signed-off-by: Israel Blancas <[email protected]>

---------

Signed-off-by: Israel Blancas <[email protected]>
rubenvp8510 pushed a commit to rubenvp8510/opentelemetry-operator that referenced this pull request May 7, 2024
* Fix truncated descriptions in the documentation

Signed-off-by: Israel Blancas <[email protected]>

* Add other generated files

Signed-off-by: Israel Blancas <[email protected]>

* Undo change in version

Signed-off-by: Israel Blancas <[email protected]>

* Add changelog

Signed-off-by: Israel Blancas <[email protected]>

* Revert changes in manifest CRDs. Add a way to generate the long descriptions in the MD

Signed-off-by: Israel Blancas <[email protected]>

* Fix changelog

Signed-off-by: Israel Blancas <[email protected]>

---------

Signed-off-by: Israel Blancas <[email protected]>
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.

Documentation for observability.metrics.enableMetrics truncated
4 participants