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.

Sign up for GitHub

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(5236): Update base docs url #15703

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(5236): Update base docs url #15703

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f834cd5
fix(5236): Update base docs url
zeeklop Jun 6, 2023
7f0b08e
fix(5236): Adds the rest of the URLs
zeeklop Jun 6, 2023
689efc8
Merge branch 'master' of github.com:Graylog2/graylog2-server into fix…
zeeklop Jun 8, 2023
bd3fc67
Merge branch 'master' of github.com:Graylog2/graylog2-server into fix…
zeeklop Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions graylog2-web-interface/src/components/common/PageHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ type Props = {
lifecycle?: 'experimental' | 'legacy',
lifecycleMessage?: React.ReactNode,
subpage: boolean,
documentationLink?: { title: string, path: string }
documentationLink?: { title: string, path: string, appVersion?: string }
};

/**
Expand All @@ -127,7 +127,7 @@ const PageHeader = ({ children, subpage, title, actions, topActions, lifecycle,
</h1>
{(documentationLink || topActions) && (
<TopActions $hasMultipleChildren={!!documentationLink && !!topActions}>
{documentationLink && <DocumentationLink text={documentationLink.title} page={documentationLink.path} displayIcon />}
{documentationLink && <DocumentationLink text={documentationLink.title} page={documentationLink.path} appVersion={documentationLink.appVersion} displayIcon />}
{topActions}
</TopActions>
)}
Expand Down
19 changes: 13 additions & 6 deletions graylog2-web-interface/src/components/support/DocumentationLink.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import * as React from 'react';
import type { ReactNode } from 'react';
import styled from 'styled-components';

import { getMajorAndMinorVersion } from 'util/Version';
import DocsHelper from 'util/DocsHelper';

import Icon from '../common/Icon';
Expand All @@ -35,18 +36,24 @@ type Props = {
page: string;
text: ReactNode;
title?: string;
appVersion?: string;
displayIcon?: boolean
}

const DocumentationLink = ({ page, title = '', text, displayIcon }: Props) => (
<Container href={DocsHelper.toString(page)} title={title} target="_blank">
{text}
{displayIcon && <StyledIcon name="lightbulb" type="regular" size="lg" />}
</Container>
);
const DocumentationLink = ({ page, title = '', text, appVersion, displayIcon }: Props) => {
const version = appVersion === 'current' ? getMajorAndMinorVersion() : appVersion;

return (
<Container href={DocsHelper.toString(page, version)} title={title} target="_blank">
{text}
{displayIcon && <StyledIcon name="lightbulb" type="regular" size="lg" />}
</Container>
);
};

DocumentationLink.defaultProps = {
title: '',
appVersion: undefined,
displayIcon: false,
};

Expand Down
13 changes: 11 additions & 2 deletions graylog2-web-interface/src/util/DocsHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,17 @@ class DocsHelper {

DOCS_URL = 'https://docs.graylog.org/docs';

toString(path) {
const baseUrl = this.DOCS_URL;
DOCS_URL_BY_VERSION = {
'5.0': 'https://go2docs.graylog.org/5-0',
// eslint-disable-next-line quote-props
'5.1': 'https://go2docs.graylog.org/5-1',
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't plan on changing the base https://go2docs.graylog.org/ would it be possible to turn this into a function that doesn't have to be updated before every release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Zack. I'm asking the docs team how do they handle the docs version. I'll update this depending on how all that works

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! This approach works, it just adds another step someone has to remember to do before an official release goes out the door.

Copy link
Contributor

@danotorrey danotorrey Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for checking on that @zeeklop. I am also wondering: would it work to parse the major and minor version numbers from the supplied appVersion and programmatically concatenate the major-minor string from that?

Copy link
Contributor Author

@zeeklop zeeklop Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@danotorrey @kingzacko1 Yes, that was my first thought. I've been talking with the docs team trying to figure it out what is the cadence of the docs. Do we get a new URL on each Graylog release, or is there a period of time when the docs are behind?

So far haven't got a straight answer. The Docs team is trying to create a URL with an alias for the latest something like

go2docs.graylog.org/latest | current/

If you look at the latest modification on this file, you'll see that what is doing is getting the mayor and minor release number and matching it with the list of docs URLs using RegEx. If it doesn't find a match, it uses the latest as base.

My concerns are

  1. If we use the concatenation approach and the docs are behind, then all links will take the user to a 404-page putting the app in a broken state until the docs catch up.
  2. Not all versions of the docs have the same base URL. 3.3 and earlier are in an archive subdomain. Looks like we started using go2docs from 4.3

In conclusion, If the Docs URL gets updated on each release, then we can use concatenation for prod with the caveat that in dev we will have to use the previous version.

Another solution would be to always send the user to the latest version, since the docs page has a dropdown to select after what version you want to see.

Trying to send the user the latest version of the docs could be a little tricky if the URL doesn't get updated during the release of a new minor or mayor version of Graylog. We would have to somehow check first that there is a version of the docs for the app version otherwise, drop a version until we find one that works (no 404)

I thought it would be an easy fix, but looks like the docs team has been at it for quite some time trying to figure it out the best solution for this.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@zeeklop Thanks for the details. Ahh, I see. the older versions use a different scheme. Given the those different schemes, I don't see a better way than the current implementation. I think this change should be helpful, since we probably have a decent portion of users on 5.0 still.

This is only applicable in cases where devs explicitly specify the appVersion, right? In that case, then the version -> docs path mapping only needs to be updated when a dev needs to reference a new specific version. The correct mapping could be added at that time, since there is a chance the scheme may change by then.

};

toString(path, inVersion = null) {
const currentDocsVersion = Object.keys(this.DOCS_URL_BY_VERSION)[Object.keys(this.DOCS_URL_BY_VERSION).length - 1];
const version = Object.keys(this.DOCS_URL_BY_VERSION).includes(inVersion) ? inVersion : currentDocsVersion;

const baseUrl = inVersion ? this.DOCS_URL_BY_VERSION[version] : this.DOCS_URL;

return path === '' ? baseUrl : `${baseUrl}/${path}`;
}
Expand Down