Skip to content

docs(install): Fix link to helm readme #3869

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

Merged
merged 1 commit into from
Jul 29, 2021
Merged

docs(install): Fix link to helm readme #3869

merged 1 commit into from
Jul 29, 2021

Conversation

greyscaled
Copy link
Contributor

Fixes #3867

@greyscaled greyscaled self-assigned this Jul 29, 2021
@greyscaled greyscaled requested a review from a team as a code owner July 29, 2021 01:59
@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #3869 (f9854b7) into main (7a5c457) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3869   +/-   ##
=======================================
  Coverage   63.40%   63.40%           
=======================================
  Files          36       36           
  Lines        1872     1872           
  Branches      379      379           
=======================================
  Hits         1187     1187           
  Misses        582      582           
  Partials      103      103           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a5c457...f9854b7. Read the comment docs.

@greyscaled greyscaled changed the title fix: Doc link to helm chart docs(install): Fix link to helm readme Jul 29, 2021
@greyscaled greyscaled enabled auto-merge (squash) July 29, 2021 02:11
@@ -234,7 +234,7 @@ alternative](https://hub.docker.com/r/linuxserver/code-server).

## Helm

You can install code-server via [Helm](../ci/helm-chart/README.md).
You can install code-server via [Helm](https://github.com/cdr/code-server/blob/main/ci/helm-chart/README.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's strange, why does this need to be an absolute link? The relative link does seem to work ok (see preview) but it links to https://coder.com/docs/code-server/v3.11.0/ci/helm-chart/README (missing the .md extension)

I don't know if something else is going on here 🤔

Copy link
Contributor Author

@greyscaled greyscaled Jul 29, 2021

Choose a reason for hiding this comment

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

Hey @jawnsy - From what I can tell, content located in ci is not rendered on coder.com.

Adding the .md extension doesn't fix this, try this link:

https://coder.com/docs/code-server/v3.11.0/ci/helm-chart/README.md

image

I think it needs to be an absolute link unless we also intend on hosting ci/helm-chart/README.md on coder.com/docs from what I see.

We seem to only host the docs/ directory (see docs/manifest.json):

https://github.com/cdr/code-server/blob/main/docs/manifest.json

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think @BrunoQuaresma knows more about this but at least we can fix this issue for now, get people to the right docs and then improve later if needed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, if we want to add it to the docs page, we have to move it to live inside of the docs folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet! made a followup issue: #3877

Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @vapurrmaid and thanks @jawnsy for asking good questions and helping review! 🙌

@greyscaled greyscaled merged commit 0283c35 into main Jul 29, 2021
@greyscaled greyscaled deleted the vapurrmaid-patch-1 branch July 29, 2021 16:54
@jsjoeio jsjoeio mentioned this pull request Jul 29, 2021
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.

Getting 404 on Helm link
4 participants