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: update changed docs link #1038

Closed
wants to merge 1 commit into from

Conversation

minaelee
Copy link

Done

  • Updates references to LXD's docs/explanation/clustering.md which has been renamed to clusters.md as part of doc: revise explanation titles lxd#14692
  • Note: a redirect was added as part of the above PR to redirect from explanation/clustering to explanation/clusters, but it doesn't prevent the lxd-ui test from failing (see below)

Fixes [list issues/bugs if needed]
Fixes this test failure: https://github.com/canonical/lxd/actions/runs/12413640553/job/34656601439

QA

Ran UI with dotrun and confirmed that links have changed.

@webteam-app
Copy link

@edlerd
Copy link
Collaborator

edlerd commented Dec 19, 2024

This is a bit tricky, because we also release the codebase for older LXD like 5.21.x, and I assume those will not receive the update of the doc link. I wonder if we should keep the old link, if with the redirect it will just work for both old and new version. The other option is to have different links for each version, but we'd need to find a good reusable solution for it. Let's discuss.

@minaelee
Copy link
Author

This is a bit tricky, because we also release the codebase for older LXD like 5.21.x, and I assume those will not receive the update of the doc link. I wonder if we should keep the old link, if with the redirect it will just work for both old and new version. The other option is to have different links for each version, but we'd need to find a good reusable solution for it. Let's discuss.

Right, the older LXD versions won't receive the update of the doc link. Keeping the old link, depending on the redirect, and only updating the expected link in tests/doc-links-spec.ts seems like it should work for this case, but it doesn't seem ideal in the long run if any other paths are changed.

I see that currently docBaseLink in ClusterList.tsx is set to useDocs(), which returns a static remoteBase of "https://documentation.ubuntu.com/lxd/en/latest" in useDocs.tsx for the main branch of lxd-ui. In the same file in the stable-5.21 branch I see that it's also set to latest there. Doesn't that mean in both versions of the UI, they point to latest docs? Is that an oversight or intended, or does it not work the way I'm thinking? First time looking at this code, so forgive me if I'm missing something obvious.

@edlerd
Copy link
Collaborator

edlerd commented Dec 19, 2024

We recently decided to stop using the stable-5.21 branch of the UI repository. So we always ship the latest tag from the main branch to future 5.21.x releases as well as to the future 6.x releases.

I see that currently docBaseLink in ClusterList.tsx is set to useDocs(), which returns a static remoteBase of "https://documentation.ubuntu.com/lxd/en/latest" in useDocs.tsx for the main branch of lxd-ui. In the same file in the stable-5.21 branch I see that it's also set to latest there. Doesn't that mean in both versions of the UI, they point to latest docs? Is that an oversight or intended, or does it not work the way I'm thinking? First time looking at this code, so forgive me if I'm missing something obvious.

Yes, your analysis is mostly correct. Since some time we ship the docs with LXD, so useDocs will return a link to the local installation of LXD, to the same domain that serves the UI. I see for 5.21 we already have these local docs enabled. That's why we'd need a different link for 5.21 as for 6.x. Ideally, we find an easy way to support docs from both versions. Would it be ok to rely on the redirect for 6.x and keep the old link? If not, we'd come up with a solution to have different links for different versions.

@minaelee
Copy link
Author

Thanks for the explanation! I don't have an issue with relying on the redirect for 6.x. Would you like me to rebase and drop the commit for ClusterList.tsx, leaving the commit for doc-links-spec.ts?

@edlerd
Copy link
Collaborator

edlerd commented Jan 6, 2025

Thanks for the explanation! I don't have an issue with relying on the redirect for 6.x. Would you like me to rebase and drop the commit for ClusterList.tsx, leaving the commit for doc-links-spec.ts?

Yes, please.

I saw in another more recent workflow run, that the doc-links-spec.ts is failing for latest/edge. This is because now the new lxd edge build is available and we pull the link from the metadata api. As the link changed there (but only for latest edge), we should adjust the test to expect the new link if we are on the new version test. In the tests, we do know which version of a backend we are testing against, you can see an example in the storage test. Though in this case you would want to compare against the latest-edge value to make the switch, as the two other cases (5.21 and 5.0) are the same.

@edlerd
Copy link
Collaborator

edlerd commented Jan 6, 2025

added this pr in the meantime, to fix the failing test: #1044

@edlerd
Copy link
Collaborator

edlerd commented Jan 7, 2025

Thanks Minnae for opening this one! Closing it, as we concluded to only fix the test (which was done in #1044) and rely on the redirect to keep the production code simple and compatible to old and new versions.

@edlerd edlerd closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants