-
Notifications
You must be signed in to change notification settings - Fork 702
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 missing usage of appVersion in a docs link #2135
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.
Thanks Antonio. See inline comment.
@@ -432,7 +435,7 @@ export function AppRepoForm({ | |||
<a | |||
target="_blank" | |||
rel="noopener noreferrer" | |||
href="https://github.com/kubeapps/kubeapps/blob/master/docs/user/private-app-repository.md#modifying-the-synchronization-job" | |||
href={`https://github.com/kubeapps/kubeapps/blob/${appVersion}/docs/user/private-app-repository.md#modifying-the-synchronization-job`} |
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 don't remember the discussion about moving the doc links to be versioned per app version - I'd personally link to the latest (master) as this was, but may have missed some context. But given that we now have an actual docs site, I'm assuming we'll want to instead link to that? So for example, this one would be: https://kubeapps.com/docs/private-app-repository/#modifying-the-synchronization-job
?
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.
While onboarding on this project, I found out that, despite every link was being versioned, these two links were not. @andresmgot suggested simply open a PR fixing this inconsistency. However, as you are pointing out, perhaps it is more interesting to change every doc link to the new documentation portal. I agree with this approach.
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.
The point of using the version in the link is that, if people are running an old version, the latest documentation may include features that are not present in the version of the user (or simply may not apply).
The nice point if we use a link to kubeapps.com is that the SEO for the page would be better but I still think that it's better from the user perspective to get the right documentation when they click on a link.
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.
So the end point there is to have versioned docs on our docs site (similar to https://goharbor.io/docs/2.1.0/ where you can select the version) and use the docs site exclusively. The question is whether in the mean-time, even though we have a docs site, we send people to the GH versions to get version-specific info (including errors which are corrected in later versions), or start sending people from the app to our docs site now (and they won't see version specific docs yet, until we fix that on the docs site).
I don't mind either way :)
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.
We don't have anything scheduled to keep working in the docs site so I don't think we will have versioned docs there any time soon. My +1 for pointing to GH.
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.
Yeah, the decision isn't about having versioned docs on our docs site now, but whether right now we send people to our (unversioned) docs site as often the updates are corrections too, or our versioned GH pages (which has version specific stuff but also uncorrected errors). Anyway, I don't have a strong pref personally. +1, thanks!
kubeappsNamespace, | ||
repo, | ||
secret, | ||
appVersion, |
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.
Rather than forwarding the appVersion
around the different components, you can just get it from the state (see the useSelector
method at line 65 in this file). You can use useSelector
to have access to the application state so just need to change that:
const {
repos: {
imagePullSecrets,
errors: { create: createError, update: updateError, validate: validationError },
validating,
},
config: {
appVersion,
},
} = useSelector((state: IStoreState) => state);
Also, you may need to update the tests for this file since the partial state
defined there won't contain the config
section.
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.
Much easier!! I'm not very used to react state yet, so I simply wasn't aware of this feature. Thanks for pointing me out!
Instead of passing trough different components, it gets the appVersion directly from state.
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
Description of the change
Currently, almost every link pointing to the Kubeapps documentation is being automatically parameterized according to the version tag. Nevertheless, a few links were not using sill this approach.
Benefits
Every link pointing to the Kubeapps documentation is now being automatically parameterized.
Possible drawbacks
I don't know yet the behavior after the PR #2129, we want
.../blob/v2.0.0/README.md
and not...blob/vv2.0.0/README.md
or.../blob/2.0.0/README.md
.Applicable issues
Additional information
N/A