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

Documentation: make the GitHub URLs depend on the current branch #1764

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Documentation: make the GitHub URLs depend on the current branch #1764

merged 1 commit into from
Oct 18, 2017

Conversation

aalemayhu
Copy link
Contributor

Using git to retrieve the current branch and then storing it in the
epilogue where it's globally available. This can later be moved should
there be a conflicts on the names. Since the base URL is a variable now
and used in the blocks, had to change to using a parsed-literal so
they are rendered. One downside to this is that we no longer get the
consistent frame around the text like the code blocks.

Closes: #1755 (Make URLs version specific to provide backwards compatibility in docs)
Signed-off-by: Alexander Alemayhu alexander@alemayhu.com

@aalemayhu aalemayhu requested review from tgraf and aanm October 16, 2017 10:53
@tgraf
Copy link
Member

tgraf commented Oct 16, 2017

@scanf Did you generate this locally with sphinx to verify it works?

@aalemayhu
Copy link
Contributor Author

@tgraf Are you seeing anything wrong with the output? Did not see any issues. Was testing it on a different server, rendered below

https://alemayhu.com/cilium-staging/gettingstarted.html#getting-started-using-kubernetes

https://alemayhu.com/c.pdf

@aalemayhu
Copy link
Contributor Author

aalemayhu commented Oct 16, 2017

Tested locally now and works for local branches (X), but not for remote branches (origin/X). So git is returning HEAD for those, but this shouldn't matter assuming the branches are locally checked out on read the docs.

@tgraf
Copy link
Member

tgraf commented Oct 16, 2017

@tgraf Are you seeing anything wrong with the output? Did not see any issues. Was testing it on a different server, rendered below

The only minor issue I can see is that the preformatted boxes are now rendered differently in my browser. The boxes with the links are rendered in a bigger font. Not an issue though.

@aalemayhu
Copy link
Contributor Author

Noticed the font and frame mismatch as well. So we are changing the default because the code directive does not parse the text and is mainly offering syntax highlighting. Not familiar with the customization options, but could take a look if you want. Maybe sphinx has some way of creating own directives that inherit from others. If that's the case we could try to inherit from parsed-literal and then try to provide a separate CSS file for the font size and border with similar values as the code directive.

@tgraf
Copy link
Member

tgraf commented Oct 17, 2017

Noticed the font and frame mismatch as well. So we are changing the default because the code directive does not parse the text and is mainly offering syntax highlighting. Not familiar with the customization options, but could take a look if you want. Maybe sphinx has some way of creating own directives that inherit from others. If that's the case we could try to inherit from parsed-literal and then try to provide a separate CSS file for the font size and border with similar values as the code directive.

I think this is fine. File a github issue with the doc tag so we don't forget about this. I think eventually we want to tackle things like these.

@aanm aanm added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. pending review labels Oct 17, 2017
@aanm
Copy link
Member

aanm commented Oct 17, 2017

@joestringer The tests failed. Do you think it is related with the L7 changes we found a week ago?

@scanf Can you rebase?

@joestringer
Copy link
Member

@aanm This failure looks the same as #1362.

@aalemayhu
Copy link
Contributor Author

@aanm Rebased

$ vim cilium.yaml
[adjust the etcd address]

After configuring the ``cilium`` ConfigMap_ it is time to deploy it using
After configuring the ``cilium`` ConfigMap it is time to deploy it using
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the link to the ConfigMap?

Using git to retrieve the current branch and then storing it in the
epilogue where it's globally available. This can later be moved should
there be a conflicts on the names.

Since the base URL is a variable now and used in the blocks, had to
change to using a `parsed-literal` so they are rendered. One downside to
this is that we no longer get the consistent frame around the text like
the code blocks.

Also fix a couple of minor issues in the install guide.

Closes: #1755 (Make URLs version specific to provide backwards compatibility in docs)
Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
@aalemayhu
Copy link
Contributor Author

aalemayhu commented Oct 18, 2017 via email

@aanm aanm requested a review from a team October 18, 2017 10:37
@tgraf tgraf merged commit 5aa04cf into cilium:master Oct 18, 2017
@aalemayhu aalemayhu deleted the links-branch branch October 18, 2017 17:06
Copy link
Contributor Author

@aalemayhu aalemayhu left a comment

Choose a reason for hiding this comment

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

Need to add this comment, so it's removed from review queue. Please ignore.

@aalemayhu aalemayhu removed the request for review from a team October 21, 2017 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants