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

docs(elastricsearch): add description fiels #9693

Merged

Conversation

dim-ops
Copy link
Contributor

@dim-ops dim-ops commented Jan 23, 2024

Add descriptions to elasticseach args

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@dim-ops dim-ops force-pushed the docs/add-elastricsearch-description branch from e8dec55 to ca86fac Compare January 23, 2024 16:51
@dim-ops dim-ops changed the title Docs/add elastricsearch description docs(elastricsearch): add description fiels Jan 23, 2024
@github-actions github-actions bot added ingestion PR or Issue related to the ingestion of metadata community-contribution PR or Issue raised by member(s) of DataHub Community labels Jan 23, 2024
)
collapse_urns: CollapseUrns = Field(
default_factory=CollapseUrns,
default_factory=CollapseUrns, description="Urn of collapse."
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case the problem seems to be in doc generation instead. CollapseUrns. urns_suffix_regex is documented properly. This documentation does not really explain the functionality.

Copy link
Contributor Author

@dim-ops dim-ops Jan 23, 2024

Choose a reason for hiding this comment

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

Same for this #9690 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That one I merged in. But urn of collapse does not really tell what this is doing. The underlying model has more details which are useful but are not showing up in the docs. If you feel comfortable reviewing the docgen code and making changes in that please do. Otherwise someone in our team can attempt that when they get a chance.

@dim-ops dim-ops force-pushed the docs/add-elastricsearch-description branch from c60f6d2 to dd51020 Compare January 26, 2024 15:07
@dim-ops dim-ops marked this pull request as draft January 26, 2024 15:07
@dim-ops dim-ops force-pushed the docs/add-elastricsearch-description branch from dd51020 to a17d3ad Compare January 26, 2024 15:13
@dim-ops dim-ops marked this pull request as ready for review January 26, 2024 15:13
@dim-ops dim-ops requested a review from anshbansal January 26, 2024 15:13
@dim-ops
Copy link
Contributor Author

dim-ops commented Feb 2, 2024

Up @anshbansal please 🙏

@anshbansal anshbansal merged commit 1d514cb into datahub-project:master Feb 5, 2024
53 checks passed
@anshbansal
Copy link
Collaborator

In the long run we would like to fix this doc generation bug. In the short term I am merging this in as I understand it can cause confusion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community ingestion PR or Issue related to the ingestion of metadata
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants