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

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

Conversation

zeeklop
Copy link
Contributor

@zeeklop zeeklop commented Jun 6, 2023

Updates how DocHelper builds the docs URL. With this update, programmers can specify the version of the docs to open when clicking on the link.

/nocl

issue: https://github.com/Graylog2/graylog-plugin-enterprise/issues/5236

Description

Updates how DocHelper builds the docs URL. With this update, programmers can specify the version of the docs to open when clicking on the link.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@zeeklop zeeklop requested review from dennisoelkers, linuspahl and a team June 6, 2023 13:38
@zeeklop zeeklop self-assigned this Jun 6, 2023
Copy link
Contributor

@ryan-carroll-graylog ryan-carroll-graylog left a comment

Choose a reason for hiding this comment

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

Tests successfully

@ryan-carroll-graylog ryan-carroll-graylog requested a review from a team June 6, 2023 14:57
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

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

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.

@linuspahl linuspahl removed their request for review July 4, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants