-
Notifications
You must be signed in to change notification settings - Fork 867
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 versioned doc support #905
Add versioned doc support #905
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a good opportunity to clean up the DocLinksService
a bit and make it clear which documentation does and does not exist. I believe in #335 we agreed to re-route "dead" links to the root documentation page for now, which we haven't done for all links, so we could fix that as well?
src/plugins/data/common/opensearch_query/kuery/node_types/wildcard.ts
Outdated
Show resolved
Hide resolved
src/plugins/data/server/autocomplete/value_suggestions_route.ts
Outdated
Show resolved
Hide resolved
Looks like there are a few linting errors. You can fix those with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few minor questions/suggestions.
// https://opensearch.org/docs/latest/opensearch/install/docker-security/ | ||
dockerSecurity: `${OPENSEARCH_VERSIONED_DOCS}install/docker-security`, | ||
// https://opensearch.org/docs/latest/opensearch/install/helm/ | ||
helm: `${OPENSEARCH_VERSIONED_DOCS}install//helm/`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo (extra /
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
}; | ||
readonly date: { | ||
readonly dateMath: string; | ||
noDocumentation: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this isn't readonly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should mark as readonly as well
src/core/public/public.api.md
Outdated
readonly management: Record<string, string>; | ||
readonly visualize: Record<string, string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why these are different from the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some unit test logic use different setting
as a key
to retrieve corresponding url. So i keep the Record type as it was.
src/core/public/public.api.md
Outdated
readonly visualize: Record<string, string>; | ||
readonly addData: string; | ||
}; | ||
readonly scriptedFields: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this exist inside of noDocumentation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I was changing the structure in doc_links_service
but forgot to copy paste here
const opensearchDashboards = useOpenSearchDashboards(); | ||
const kueryQuerySyntaxDocs = opensearchDashboards.services.docLinks!.links.query.kueryQuerySyntax; | ||
const docLinks = useOpenSearchDashboards().services.docLinks; | ||
const kueryQuerySyntaxDocs = docLinks!.links.opensearchDashboards.dql.base; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we take the opportunity to rename these keury
variables now as referenced in #769?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will update
const opensearchDashboadsDoc = opensearchDashboards.services.docLinks!.links.opensearchDashboards | ||
.introduction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to add a vega
property to the noDocumentation
object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can.
@@ -141,7 +141,10 @@ function DateRangesParamEditor({ | |||
<EuiFormRow compressed fullWidth> | |||
<> | |||
<EuiText size="xs"> | |||
<EuiLink href={services.docLinks.links.date.dateMath} target="_blank"> | |||
<EuiLink | |||
href={services.docLinks.links.opensearch.aggregations.bucket.range} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the right link? bucket aggregations are a different thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so sure, I was thinking to use : https://opensearch.org/docs/latest/opensearch/units/ but it doesn't contains enough info related date_range
. Instead, https://opensearch.org/docs/latest/opensearch/bucket-agg/#range-date_range-ip_range, contains more info about date_range
. Open to any inputs
Like the proposal, do we know if custom plugins consume the docs service? Like I would check some of the dashboards plugins in the Also, have you verified the 'zero-data' state works fine. I pulled this down and works fine with me just haven't checked zero data and thought I'd ask before clearing my stuff out. NIT: Can you squash the commits with the context provided? Thanks! |
Yes, I have tested the app with no data ingested, the urls look fine to me. My understanding is that all the urls in To make sure my understanding regarding the comments is correct, you are suggesting to use :
Will squash commits in next revision |
For sure, just making sure if there were links on views that only showed in the empty data state existed then there are still working.
Not sure, if it should go to Query DSL, seeing more if it should go the newly created to the new DQL link created.
If dateMath doesn't exist than 10-4 because just looking in the diff going from
💯 Any comment about custom plugins or still verifying that? |
Need some time to check the plugins |
By searching the code references in other open search plugin repos, there is no plugin depended on |
7242baf
to
33cd5e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since adding a conditional we should probably had a test or two for verifying the conditional. Otherwise LGTM. I'm in an opinion this doesn't get backported to 1.2 and hesitatant to backport to 1.x since we are restructuring things. I don't think external plugins would be consuming this but just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I noticed we have the path src/plugins/data/common/opensearch_query/kuery
. We should probably rename this. It doesn't have to be in this PR so we don't pollute the commits but it should probably be done
Agreed on all counts. Can we spend a little more time on adding tests? |
Yeah. Working on adding the new tests |
33cd5e8
to
a1f2927
Compare
Is this the case for any service in our repo? If so, that's a serious inhibitor for any change that we're making. Not being able to refactor anything between major version bumps is a slippery slope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Just a couple very minor changes and then I think we're good to go.
src/plugins/data/public/ui/query_string_input/language_switcher.tsx
Outdated
Show resolved
Hide resolved
a1f2927
to
7c1c09a
Compare
This is PR is to add versioned document support in OSD. 1. Add logic to pick up doc version from package.json and convert it to `latest` if we are on default `main` branch. 2. Refactor doc_link_service to have 3 urls groups: opensearch, opensearchDashboards, and noDocumentation. 3. Update dynamic versioned doc links and clean up unused urls 4. Fix known url bug opensearch-project#769 5. Add unit tests for doclinks branch name conversion Signed-off-by: Zuocheng Ding <zding817@gmail.com>
7c1c09a
to
6b5d287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
…rch-project#905) bump semantic-release from 16.0.1 to 17.2.3 and upgrade semantic-release deps and node version Bumps [semantic-release](https://github.com/semantic-release/semantic-release) from 16.0.1 to 17.2.3. - [Release notes](https://github.com/semantic-release/semantic-release/releases) - [Commits](semantic-release/semantic-release@v16.0.1...v17.2.3) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>
Description
This PR is to add versioned doc url support in DocLinksServices.
package.json
to control the doc url versioning. If thebranch
value ismain
then convert it tolatest
as the document version, otherwise use thebranch
value as the document version (e.g.1.1
, or1.0
)opeansearch
,opensearchDashboards
andnoDocuments
. Mapping the structure into: [Group] -> [sub-page]->[sub-subpage] -> [anchor].Issues Resolved
Check List