Fpetrilli/improve docs #40

FrankPetrilli · 2018-01-13T23:54:21Z

…ples.

ZenGround0 · 2018-01-14T01:49:45Z

docs/test_config.md

+
+Sometimes, we want to scale up or down our tests to compare against the size of a given architecture.
+
+To this end, we can make use of the config.yml to define certain constants we wish to make use of. For an example of how to use this in a real test, see `config-writer.sh`, which creates the config.yml file from the user's input to automatically scale the tests.


Consider avoiding double "make use of" in the first sentence to keep the phrasing crisp.

While this is a good first stab it doesn't communicate the purpose or functionality of the config accurately enough. While currently we do use the config to specify the number of nodes in the cluster, we also use it to specify the number of pins that certain tests make (see param Y). In general the config is good for substituting any variables across tests. For more info look into @dgrisham's PR introducing it.

ZenGround0 · 2018-01-14T01:57:27Z

docs/test_config.md

+
+To this end, we can make use of the config.yml to define certain constants we wish to make use of. For an example of how to use this in a real test, see `config-writer.sh`, which creates the config.yml file from the user's input to automatically scale the tests.
+
+```


It is a bit unclear that this statement goes in a config file. You should briefly hit on the config file's format and how it is activated (i.e. flag specifying path to kubernetes-ipfs, same folder as tests) too.

ZenGround0 · 2018-01-14T02:06:14Z

docs/selectors.md

@@ -0,0 +1,14 @@
+# Selectors


Nice addition. Looking at this alongside other docs it's becoming clear the file organization I began here is not ideal. Right now its unclear what files are documenting what. This documentation has a different context from that documenting DSL features and our docs should convey that. I'm thinking either one large file with broader subsections (kubernetes-ipfs commands, The DSL, Kubernetes Config, etc) or perhaps separate files for each broader subsection.

On that note it would be good to have more background on the kubernetes config so that this section will make sense to people without a kubernetes background.

@FrankPetrilli both of these comments should be addressed before merging:

Consolidate docs into one file or a few logically laid out files to make documentation more approachable

Preface this specific section with background on the kubernetes config file that the selector key value pair goes into. Could be as simple as a link to existing kubernetes docs or a few concise sentences following a header.

Okay, w.r.t. number 2, are you asking for an explanation of where labels go in a kubernetes yaml definition and their purpose?

That sounds along the right track. I think the best answer is that I don't have the kubernetes context that you do to get much more specific. The labels exist in a file that I don't know much about. It would be great if the docs introduced me to this section without assuming I knew what that file is called, what it is used for and what its format is. imo the best way to do this is with the structure suggested in (1) including this section as a subsection of "the kubernets yaml definition"

or a name you think fits better :)

ZenGround0 · 2018-01-14T02:15:26Z

docs/validation.md

+With in the confines of a 'step', adding the 'outputs' parameter allows the user to send the output from the command on a given line to a given name.
+This can then be verified later to validate the output of a particular command.
+
+The format is for a given `line`, save the output to the named variable `save_to`.


You should try to find a way to incorporate the append_to feature in this discussion too, perhaps referencing the iteration section.

ZenGround0 · 2018-01-14T02:20:39Z

docs/validation.md

+
+Once the successes and failures have been talied up across runs for a particular test, the observed value is compared against the `expected` block.
+If the observed value matches, `kubernetes-ipfs` will report that expectations were met, and returns a 0 exit code. If they don't match, it will return
+a non-zero exit code, and state that the expectations were not met. This allows users to short-circut the tests and immediately fail in the event of a test failure


What do you mean by short-circuit the tests? Please be a little more specific here. It's been a while but I recall all steps being run even if an early step failed an assertion. Are you talking about short circuiting the times loop?

Okay, I've attempted to address all your major comments in the latest changes, but this one I'm having some trouble trying to word.

What I'm trying to say is that since we run kubernetes-ipfs against a "test", when a test fails due to not matching expected values, one can take that into account by using its return value in a loop, something like:

for file in $(find tests -not -name 'config.yml' -name '*.yml' | sort) do kubernetes-ipfs $file if [ $? -ne 0 ]; then echo "FAILED ON $file" break fi done

ooh I see. In that case you're fine dropping the short circuit terminology altogether. I am assuming people reading these docs understand the benefit of setting exit codes.

FrankPetrilli · 2018-01-18T18:13:39Z

Okay, @ZenGround0, I think this covers everything you wanted changed. Thoughts?

ZenGround0 · 2018-01-18T18:09:11Z

docs/selectors.md

@@ -0,0 +1,14 @@
+# Selectors


@FrankPetrilli both of these comments should be addressed before merging:

Consolidate docs into one file or a few logically laid out files to make documentation more approachable

Preface this specific section with background on the kubernetes config file that the selector key value pair goes into. Could be as simple as a link to existing kubernetes docs or a few concise sentences following a header.

ZenGround0 · 2018-01-18T18:12:10Z

docs/selectors.md

+
+Sometimes, we want to run a particular test only against certain subsets of pods in the Kubernetes cluster (as we may be running multiple simultaneous tests together).
+
+For this purpose, one can make use of the `selector` attribute under a test definition. This follows the kubernetes format of `key=value`, where some of the most common are the `run` key, which indicates a grouping of pods that are part of a particular rungroup.


A good example of how the above suggestion will improve readability: by spending some time explaining the kubernetes config format before diving into the specifics of the selector section, you will not need to make a digression explaining the general format and its common keys in the middle of explaining the selector attribute.

ZenGround0 · 2018-01-18T18:13:59Z

docs/test_config.md

+- A config file will be automatically found and used if named `config.yml` and in the same directory as the tests
+- Specify the path to the config using the flag `--config`.
+
+```


This will be more readable if you specify that this snippet belongs in config.yml

ZenGround0 · 2018-01-18T18:20:49Z

docs/test_config.md

@@ -0,0 +1,24 @@
+# Test config
+
+Sometimes, we want to scale up or down our tests to compare against the size of a given architecture.


While you have added some text incorporating the suggested change this line should be updated too. It is misleading as the opener of the test config section as it's not just variables that track with the size of an architecture that the test configuration allows a user to control (ex: expected successes, number of iterations)

ZenGround0 · 2018-01-18T18:23:14Z

docs/validation.md

+
+## Outputs
+
+With in the confines of a 'step', adding the 'outputs' parameter allows the user to send the output from the command on a given line to a given name.


small typo: "with in" -> "within"

ZenGround0 · 2018-01-18T18:28:28Z

@FrankPetrilli, still a few more things before this is ready to go. Let me know if you have any questions (feel free to ask in the comments, and tag my name so I don't miss it)

FrankPetrilli added 3 commits January 12, 2018 14:05

Working on #245, major DSL features documented and given minimal exam…

cad112c

…ples.

Working on ipfs-cluster/ipfs-cluster#245, cleaning up existing

3461f41

Working on ipfs-cluster/ipfs-cluster#245, cleaning up existing round 2

0b4a7ce

FrankPetrilli requested review from hsanjuan and ZenGround0 January 13, 2018 23:54

ZenGround0 suggested changes Jan 14, 2018

View reviewed changes

FrankPetrilli added 2 commits January 15, 2018 05:33

Improve documentation further given @ZenGround0's comments on /pull/40

01a7d90

Removed short-circuit description on advice of @ZenGround0

1d62ded

ZenGround0 suggested changes Jan 18, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fpetrilli/improve docs #40

Fpetrilli/improve docs #40

FrankPetrilli commented Jan 13, 2018

ZenGround0 Jan 14, 2018

ZenGround0 Jan 14, 2018

ZenGround0 Jan 14, 2018

ZenGround0 Jan 14, 2018

ZenGround0 Jan 14, 2018

ZenGround0 Jan 18, 2018

FrankPetrilli Jan 18, 2018

ZenGround0 Jan 18, 2018

ZenGround0 Jan 18, 2018

ZenGround0 Jan 14, 2018

ZenGround0 Jan 14, 2018

FrankPetrilli Jan 15, 2018

ZenGround0 Jan 15, 2018

FrankPetrilli commented Jan 18, 2018

ZenGround0 Jan 18, 2018

ZenGround0 Jan 18, 2018

ZenGround0 Jan 18, 2018

ZenGround0 Jan 18, 2018

ZenGround0 Jan 18, 2018

ZenGround0 commented Jan 18, 2018


		Sometimes, we want to scale up or down our tests to compare against the size of a given architecture.

		To this end, we can make use of the config.yml to define certain constants we wish to make use of. For an example of how to use this in a real test, see `config-writer.sh`, which creates the config.yml file from the user's input to automatically scale the tests.


		Sometimes, we want to run a particular test only against certain subsets of pods in the Kubernetes cluster (as we may be running multiple simultaneous tests together).

		For this purpose, one can make use of the `selector` attribute under a test definition. This follows the kubernetes format of `key=value`, where some of the most common are the `run` key, which indicates a grouping of pods that are part of a particular rungroup.

		@@ -0,0 +1,24 @@
		# Test config

		Sometimes, we want to scale up or down our tests to compare against the size of a given architecture.


		## Outputs

		With in the confines of a 'step', adding the 'outputs' parameter allows the user to send the output from the command on a given line to a given name.

Fpetrilli/improve docs #40

Are you sure you want to change the base?

Fpetrilli/improve docs #40

Conversation

FrankPetrilli commented Jan 13, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankPetrilli commented Jan 18, 2018

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

ZenGround0 commented Jan 18, 2018