-
Notifications
You must be signed in to change notification settings - Fork 27
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 reference to env variables & tokens #177
base: master
Are you sure you want to change the base?
add reference to env variables & tokens #177
Conversation
I've had trouble saving my comments as there seems to be a possible service outage. I'll have another go at leaving feedback tomorrow. |
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.
Some more feedback.
@@ -0,0 +1,70 @@ | |||
--- | |||
title: "Environment Variables" |
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 use sentence case for all OpenCue documentation, so this should be:
Environment variables
@@ -0,0 +1,70 @@ | |||
--- | |||
title: "Environment Variables" | |||
linkTitle: "Environment Variables" |
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.
Environment variables
date: 2019-03-30 | ||
weight: 4 | ||
description: > | ||
Reference for common environment variables |
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 recommend making this more task oriented, as follows:
Configure common environment variables
--- | ||
title: "Environment Variables" | ||
linkTitle: "Environment Variables" | ||
date: 2019-03-30 |
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 personally see much value in merging a page like this has no useful information. The tokens page looks useful.
However if we merge the environment variables page, then we should set the page in the future so that our publishing framework (Hugo) doesn't actually make the page visible until it's ready.
For example set the date to: 2025-01-01
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.
@sharifsalah You can let me know the useful information needed to be added in the environment variable's page and I will add it accordingly.
I included the page as it was mentioned in the issue but I don't exactly know the description of each variable required for documentation.
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 think it's ok to submit it with a future publish-date, then I can send a followup PR to fill in the content for each env var.
content/docs/Reference/tokens.md
Outdated
Various tokens that can be used in the commands you send to Cue | ||
--- | ||
|
||
Tokens can be used in the commands when submitting outline jobs, either in the graphical Cuesubmit client, or via the PyOutline API. |
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.
Please break up long lines of text to avoid wrapping. I usually use a 100-char limit on lines.
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.
Done. please review the PR now.
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've left a few additional comments for the tokens doc. The dates look good to me. Thanks @apoorvkhare07
content/docs/Reference/tokens.md
Outdated
date: 2020-04-03 | ||
weight: 5 | ||
description: > | ||
Various tokens that can be used in the commands you send to Cue |
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 recommend updating the description to:
Specify dynamic tokens in OpenCue jobs
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.
To be clear, here I'm just describing the value of the string to update. I'm not describing the syntax or the way you should update the code. If you looks at similar Markdown (.md
) files, you should be able to work out how to format the description but let us know if you're stuck.
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.
Oh ya, sorry my bad. I didn't pay attention to that.
Will fix this
content/docs/Reference/tokens.md
Outdated
Tokens can be used in the commands when submitting outline jobs, either in the graphical Cuesubmit | ||
client, or via the PyOutline API. | ||
|
||
Following are the commonly used tokens: |
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 recommend updating to:
This page describes tokens you can use in commands when submitting OpenCue jobs, either in the graphical Cuesubmit
client, or using the PyOutline API.
(I would delete the sentence 'Following are the commonly used tokens:' as we're not
introducing a list)
@sharifsalah @bcipriano I only added changes suggested by @sharifsalah in the token docs in the last commit but there are checks which are failing related to deploy preview. Can you help me with this ? |
@apoorvkhare07 you can review the build log by clicking Details next to any of the checks. In this case the deploy log is at: https://app.netlify.com/sites/elated-haibt-1b47ff/deploys/5e8f4b53a0852000066e46bd It looks like the relevant error is:
So it looks like you have a syntax error in the nested YAML portion of the file. I suspect you need to fix the indentation of the description. The YAML specifications are sensitive about indentation. I think this should be very easy for you to fix if you match the indentation and white space for |
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.
Looks good @apoorvkhare07, thank you.
Over to @bcipriano |
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. We can fill in the rest of the content later.
This PR initiates work on #131 by adding reference documentation for commonly used environment variables and tokens.