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

add reference to env variables & tokens #177

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

apoorvkhare07
Copy link
Contributor

This PR initiates work on #131 by adding reference documentation for commonly used environment variables and tokens.

content/docs/Reference/env-variables.md Outdated Show resolved Hide resolved
content/docs/Reference/env-variables.md Outdated Show resolved Hide resolved
content/docs/Reference/env-variables.md Outdated Show resolved Hide resolved
content/docs/Reference/tokens.md Outdated Show resolved Hide resolved
content/docs/Reference/tokens.md Outdated Show resolved Hide resolved
content/docs/Reference/tokens.md Outdated Show resolved Hide resolved
content/docs/Reference/tokens.md Outdated Show resolved Hide resolved
content/docs/Reference/tokens.md Outdated Show resolved Hide resolved
@sharifsalah
Copy link
Collaborator

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.

Copy link
Collaborator

@sharifsalah sharifsalah left a 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"
Copy link
Collaborator

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"
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

@sharifsalah sharifsalah left a 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

date: 2020-04-03
weight: 5
description: >
Various tokens that can be used in the commands you send to Cue
Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

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:
Copy link
Collaborator

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)

@apoorvkhare07
Copy link
Contributor Author

@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 ?

@sharifsalah
Copy link
Collaborator

@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:

5:21:19 PM: Error: Error building site: "/opt/build/repo/content/docs/Reference/tokens.md:8:1": failed to unmarshal YAML: yaml: line 7: could not find expected ':'

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 env-variables.md, which isn't throwing an error.

Copy link
Collaborator

@sharifsalah sharifsalah left a 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.

@sharifsalah
Copy link
Collaborator

Over to @bcipriano

Copy link
Collaborator

@bcipriano bcipriano left a 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.

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