docs: Add document for environment variables #5080

terrytangyuan · 2021-02-10T21:55:36Z

Currently we have the use of environment variables in many places at different levels but there's no documentation on them. This PR takes an initial stab at documenting them.

cc @alexec @simster7

Signed-off-by: terrytangyuan [email protected]

Checklist:

My organization is added to USERS.md.

simster7

It'd be awesome if this could be auto-generated by looking at source files. This would also force the documentation to appear in someway in the location where it is used. Do you think you could manage a script to do that @terrytangyuan ?

simster7 · 2021-02-10T22:14:20Z

Similar to how all of our other docs are generated (fields.md from *_types.go, etc.)

alexec · 2021-02-10T22:22:58Z

docs/environment-variables.md

@@ -0,0 +1,46 @@
+# Environment Variables
+
+This document outlines the set of environment variables that can be set to customize the behaviours at different levels.


It needs to be clearly stated that these are unsupported and may be removed.

Good point. Just added a note on this.

terrytangyuan · 2021-02-11T01:48:32Z

It'd be awesome if this could be auto-generated by looking at source files. This would also force the documentation to appear in someway in the location where it is used. Do you think you could manage a script to do that @terrytangyuan ?

@simster7 Yes that's definitely achievable. However I think it's a bit overkill and it doesn't seem to require a lot of efforts maintaining this document at this point. I agree that in the long term it makes sense to automate the process as the list grows so it's easy to maintain but perhaps low priority for now.

alexec · 2021-02-11T02:22:38Z

docs/environment-variables.md

+| `REMOVE_LOCAL_ART_PATH` | bool | Whether to remove local artifacts. |
+
+
+## CLI


I think can exclude CLI ones, you can run argo --help

Sure. Removed this section.

alexec · 2021-02-11T02:48:50Z

docs/environment-variables.md

@@ -0,0 +1,36 @@
+# Environment Variables
+
+This document outlines the set of environment variables that can be used to customize the behaviours at different levels.


I think I'm wrong about the text for this. I think we should not emphasize they are unsupported, more that they are typically added to test out experimental features and should not be needed by most users.

That sounds much better actually. I just pasted the exact sentence you said but feel free to modify further.

I would actually explicitly say that these may be removed at any time

alexec · 2021-02-11T16:26:06Z

@simster7 before merge, you always have valuable opinions on docs, do you want to take a look?

simster7

I still believe that this should be auto generated, or at the very least it should have an automatic check that all of the flags here still exist in the codebase and that no flags that exist in the code base are not here. Otherwise this file would in practice get neglected and become out of date soon.

This could be done with a simple regex:

simster7 · 2021-02-11T16:28:28Z

docs/environment-variables.md

@@ -0,0 +1,36 @@
+# Environment Variables
+
+This document outlines the set of environment variables that can be used to customize the behaviours at different levels.


I would actually explicitly say that these may be removed at any time

simster7 · 2021-02-11T16:29:01Z

docs/environment-variables.md

+|----------|------|------------|
+| `ALL_POD_CHANGES_SIGNIFICANT` | bool |  Whether to consider all pod changes as significant during pod reconciliation. |
+| `ALWAYS_OFFLOAD_NODE_STATUS` | bool | Whether to always offload the node status. |
+| `ARCHIVED_WORKFLOW_GC_PERIOD` | time.duration | The periodicity for GC of archived workflows. |


simster7 · 2021-02-11T16:29:57Z

docs/environment-variables.md

+
+| Name | Type | Description|
+|----------|------|------------|
+| `ALL_POD_CHANGES_SIGNIFICANT` | bool |  Whether to consider all pod changes as significant during pod reconciliation. |


I would also wrap types in in-line codeblocks to be consistent with our docs elsewhere

Suggested change

| `ALL_POD_CHANGES_SIGNIFICANT` | bool | Whether to consider all pod changes as significant during pod reconciliation. |

| `ALL_POD_CHANGES_SIGNIFICANT` | `bool` | Whether to consider all pod changes as significant during pod reconciliation. |

terrytangyuan · 2021-02-11T20:11:06Z

@simster7 Thanks! Just addressed the comments.

I probably don't have width to work on automating this at this point yet. Anyone else please feel free to contribute that before I get to it. Just in case someone else might be working on this, note that besides env.LookupEnv** from our own utils (which we only have a few that uses those), there are also uses of os.LookupEnv, os.Getenv, etc. that should be taken into account when writing the automation code.

.

simster7 · 2021-02-12T14:46:26Z

docs/environment-variables.md

+| `LEADER_ELECTION_IDENTITY` | `string` | The ID used for workflow controllers to elect a leader. |
+| `MAX_OPERATION_TIME` | `time.Duration` | The maximum time a workflow operation is allowed to run for before requeuing the workflow onto the work queue. |
+| `OFFLOAD_NODE_STATUS_TTL` | `time.Duration` | The TTL to delete the offloaded node status. Currently only used for testing. |
+| `POD_GC_TRANSIENT_ERROR_MAX_REQUEUES` | `int` | The maximum number of requeues of pods in the GC queue when encountering transient errors. |


Actually, can you remove this and add it in #5060 so that it doesn't fail the check when I merge #5091?

Removed and rebased.

I'll add this in #5060 once this has been merged.

Signed-off-by: terrytangyuan <[email protected]>

simster7 reviewed Feb 10, 2021

View reviewed changes

alexec reviewed Feb 10, 2021

View reviewed changes

alexec reviewed Feb 11, 2021

View reviewed changes

alexec approved these changes Feb 11, 2021

View reviewed changes

simster7 previously requested changes Feb 11, 2021

View reviewed changes

alexec approved these changes Feb 12, 2021

View reviewed changes

simster7 suggested changes Feb 12, 2021

View reviewed changes

terrytangyuan added 6 commits February 12, 2021 09:59

docs: Add document for environment variables

9cfbb4e

Signed-off-by: terrytangyuan <[email protected]>

Add note

0bf809a

Signed-off-by: terrytangyuan <[email protected]>

Remove CLI section

a32153c

Signed-off-by: terrytangyuan <[email protected]>

Rephrase

c40dbe3

Signed-off-by: terrytangyuan <[email protected]>

Address comments

0b55b56

Signed-off-by: terrytangyuan <[email protected]>

Remove pod gc env var

e86a837

Signed-off-by: terrytangyuan <[email protected]>

terrytangyuan requested a review from simster7 February 12, 2021 14:59

simster7 approved these changes Feb 12, 2021

View reviewed changes

simster7 merged commit cda5dc2 into argoproj:master Feb 12, 2021

terrytangyuan deleted the env-vars-doc branch February 12, 2021 15:12

simster7 mentioned this pull request Feb 16, 2021

v2.12.9 cherry-pick #5119

Closed

33 tasks

simster7 mentioned this pull request Feb 23, 2021

Cherry-pick for v2.12.10 #5179

Closed

34 tasks

simster7 mentioned this pull request Mar 8, 2021

v2.12.10 cherry-pick #5326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Add document for environment variables #5080

docs: Add document for environment variables #5080

terrytangyuan commented Feb 10, 2021

simster7 left a comment

simster7 commented Feb 10, 2021

alexec Feb 10, 2021

terrytangyuan Feb 11, 2021

terrytangyuan commented Feb 11, 2021 •

edited

Loading

alexec Feb 11, 2021

terrytangyuan Feb 11, 2021

alexec Feb 11, 2021

terrytangyuan Feb 11, 2021

simster7 Feb 11, 2021

alexec commented Feb 11, 2021

simster7 left a comment

simster7 Feb 11, 2021

simster7 Feb 11, 2021

simster7 Feb 11, 2021

terrytangyuan commented Feb 11, 2021 •

edited

Loading

simster7 Feb 12, 2021

terrytangyuan Feb 12, 2021

terrytangyuan Feb 12, 2021

		@@ -0,0 +1,46 @@
		# Environment Variables

		This document outlines the set of environment variables that can be set to customize the behaviours at different levels.

		\| `REMOVE_LOCAL_ART_PATH` \| bool \| Whether to remove local artifacts. \|


		## CLI

		@@ -0,0 +1,36 @@
		# Environment Variables

		This document outlines the set of environment variables that can be used to customize the behaviours at different levels.

	\| `ARCHIVED_WORKFLOW_GC_PERIOD` \| time.duration \| The periodicity for GC of archived workflows. \|
	\| `ARCHIVED_WORKFLOW_GC_PERIOD` \| time.Duration \| The periodicity for GC of archived workflows. \|

	\| `ALL_POD_CHANGES_SIGNIFICANT` \| bool \| Whether to consider all pod changes as significant during pod reconciliation. \|
	\| `ALL_POD_CHANGES_SIGNIFICANT` \| `bool` \| Whether to consider all pod changes as significant during pod reconciliation. \|

docs: Add document for environment variables #5080

docs: Add document for environment variables #5080

Conversation

terrytangyuan commented Feb 10, 2021

simster7 left a comment

Choose a reason for hiding this comment

simster7 commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan commented Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec commented Feb 11, 2021

simster7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan commented Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan commented Feb 11, 2021 •

edited

Loading

terrytangyuan commented Feb 11, 2021 •

edited

Loading