Revamp CONTRIBUTING.md [SKIP CI] #1495

shuklanirdesh82 · 2017-06-27T22:32:28Z

Fixes #1399 (Revamping CONTRIBUTING.md)

Refreshing test setup requirement and bring contributing.md to current.

shaominchen · 2017-06-27T22:47:08Z

CONTRIBUTING.md

@@ -177,7 +176,7 @@ If you want to user another DockerHub repository you need to set `DOCKER_HUB_REP
  - `GOVC_URL` same as `ESX IP`
  - `GOVC_USERNAME` & `GOVC_PASSWORD`: user credentials logging in to `ESX IP`

- You **need** to set following environment variables to run swarm cluster related tests.
+- You **need** to set following environment variables to run swarm cluster related tests. You **need** to configure swarm cluster inorder to run swarm related testcase otherwise there will be a test failure.


Nit: inorder => in order

shaominchen · 2017-06-27T22:47:55Z

CONTRIBUTING.md

@@ -196,6 +195,7 @@ or
 export ESX=10.20.105.54
 export VM1=10.20.105.121
 export VM2=10.20.104.210
+export VM2=10.20.104.241


shaominchen

LGTM.

msterin · 2017-06-28T20:12:46Z

CONTRIBUTING.md

@@ -146,9 +146,9 @@ of binaries in ./build directory.

 In order to test locally, you'd need a test setup. Local test setup automation is planned but but not done yet, so currently the environment has to be set up manually.



Please add a subheader for "Local Test bed setuop" , currently it's mixed up with other stuff

msterin · 2017-06-28T20:13:29Z

CONTRIBUTING.md

@@ -166,8 +166,7 @@ Makefile targets. There targets rely on environment variables to point to the
 correct environment.

 Environment variables:
- You **need** to set ESX and either VM_IP (in which case we'll use 1 VM) or
-both VM1 and VM2 environment variables
+- You **need** to set ESX and VM1, VM2 and VM3 environment variables


and if I don't , will the test tell me that ? It has to IMO, it is super cheap to check for .At the very least, tell how the failure will look like if the vars are not set up

Yeah the error message improvement will be handled separately in the different PR. More detailed information is added.

msterin · 2017-06-28T20:14:25Z

CONTRIBUTING.md

-Test environment  typically consist of 1 ESX and 2  guest VMs running inside of the
-ESX. We also support 1 ESX and 1 guest VM. We require ESX 6.0 and later,
-and a Linux VM running  Docker 1.10+ enabled for  plain text TCP connection, i.e.
+Test environment  typically consist of 1 ESX and 3 guest VMs running inside of the


local test bed is getting more and more complicated to set up :-(
There should be a way to do basic tests on a single VM. (not related to documentation)

Agree with Mark. Nirdesh and I discussed this and we agree it's too heavy to have the basic precheckin tests running on a swarm cluster (at least 3 nodes) environment. For internal developers, we can use nimbus to deploy a testbed with one command. But for external developers, it will be too complicated to set up the environment simply for running the precheckin test.

On the other hand, we also want to make sure our CI can capture most common issues by running all P0 and part of P1 test cases. Also we should still support running all complicated test cases locally with some optional parameters

To achieve these different goals, here's our plan:

Developers should be able to run test-all on a single VM. This is achieved by moving all extensive test cases (which require complicated setup) to one or more special labels

Developers should be able to run complicated test case by specifying some optional parameters. The assumption is that the environment should be ready for running those tests

CI should continue to run all extensive test cases as we have already planned

Let me know if you have any comments @msterin @shuklanirdesh82 . I will file a issue to track this task.

Filed issue #1505

msterin · 2017-06-28T20:16:08Z

CONTRIBUTING.md

@@ -177,7 +176,7 @@ If you want to user another DockerHub repository you need to set `DOCKER_HUB_REP
  - `GOVC_URL` same as `ESX IP`
  - `GOVC_USERNAME` & `GOVC_PASSWORD`: user credentials logging in to `ESX IP`

- You **need** to set following environment variables to run swarm cluster related tests.
+- You **need** to set following environment variables to run swarm cluster related tests. You **need** to configure swarm cluster in order to run swarm related testcase otherwise there will be a test failure.


Oh, so now I need 3 VMS and a swarm cluster? Or do I need to configure these VMs into a cluster?
Please provide instructions or reference to configure swarm too.

Do I need to configure any env for SWARM ? MASTER ? SLAVE(s) ?

I have added a TODO information ... and will work on simplifying the make test-all invocation. It will be handled separately in different PR having it said keeping this PR only for documentation changes.

pshahzeb

Minor comments.
Since we are updating, I think we should add a couple of lines in to document current know issues with our CI that we aren't fixing and if someone faces them, they need to restart the CI.

pshahzeb · 2017-06-30T02:58:07Z

CONTRIBUTING.md

-Test environment  typically consist of 1 ESX and 2  guest VMs running inside of the
-ESX. We also support 1 ESX and 1 guest VM. We require ESX 6.0 and later,
-and a Linux VM running  Docker 1.10+ enabled for  plain text TCP connection, i.e.
+**TODO**: As a next step, an approach will be provided to run selective test and above testbed requirement will be simplified. Basic test run will not be block when minimum testbed requirement is met i.e. 1 ESX and 1 or 2 VMs.


will not be block -> will not be blocked

pshahzeb · 2017-06-30T02:58:37Z

CONTRIBUTING.md

-and a Linux VM running  Docker 1.10+ enabled for  plain text TCP connection, i.e.
+**TODO**: As a next step, an approach will be provided to run selective test and above testbed requirement will be simplified. Basic test run will not be block when minimum testbed requirement is met i.e. 1 ESX and 1 or 2 VMs.
+
+**Note**: All 3 guest VMs can be resued to run swarm testcase by configuring swarm cluster (by making one node as master and registered others as worker nodes). It is not mandatory to create extra VM to cofigure swarm cluster but feel free if you would like to do so. Please make sure to set environment variables correctly as mentioned in section below.


cofigure -> configure

ashahi1 · 2017-06-30T03:19:10Z

CONTRIBUTING.md

-and a Linux VM running  Docker 1.10+ enabled for  plain text TCP connection, i.e.
+**TODO**: As a next step, an approach will be provided to run selective test and above testbed requirement will be simplified. Basic test run will not be block when minimum testbed requirement is met i.e. 1 ESX and 1 or 2 VMs.
+
+**Note**: All 3 guest VMs can be resued to run swarm testcase by configuring swarm cluster (by making one node as master and registered others as worker nodes). It is not mandatory to create extra VM to cofigure swarm cluster but feel free if you would like to do so. Please make sure to set environment variables correctly as mentioned in section below.


resued -> reused

ashahi1 · 2017-06-30T03:20:21Z

CONTRIBUTING.md

@@ -177,7 +116,8 @@ If you want to user another DockerHub repository you need to set `DOCKER_HUB_REP
  - `GOVC_URL` same as `ESX IP`
  - `GOVC_USERNAME` & `GOVC_PASSWORD`: user credentials logging in to `ESX IP`

- You **need** to set following environment variables to run swarm cluster related tests.
+- You **need** to set following environment variables to run swarm cluster related tests. You **need** to configure swarm cluster in order to run swarm related testcase otherwise there will be a test failure. As mentioned above, you may reuse the same set of VMs here no need to create separately.
+


here no -> here, no

ashahi1 · 2017-06-30T03:29:33Z

CONTRIBUTING.md

+* [Cutting a new release guidelines](#cutting-a-new-release-guidelines)
+  * [Tagging a release](#tag-a-release)
+  * [Generating change log](#generate-the-change-log)
+  * [Publishing artifects](#publish-vdvs-managed-plugin-to-docker-store)


Please check if it is artifects or artifacts?

pshahzeb

LGTM

shaominchen

LGTM. A few nits below.

shaominchen · 2017-06-30T20:07:46Z

CI.md

+  - Due to security concerns the artifacts will not be published.
+    https://github.com/drone/drone/issues/1476
+
+- Each commit into the master operation will run the full set of tests


Nits - the sentence seems not very smooth:

Each commit into the master operation => Each commit into the master branch?

run the full set of tests part of... => run the full set of tests as part of...?

shaominchen · 2017-06-30T20:10:26Z

CI.md

+
+## CI System
+
+We are using the CI system that has been up by the CNA folks (@casualjim, @frapposelli & @mhagen-vmware).


Nit: has been up by => has been supported by?

shaominchen · 2017-06-30T20:10:47Z

CI.md

+
+We are using the CI system that has been up by the CNA folks (@casualjim, @frapposelli & @mhagen-vmware).
+The CI system is based on https://drone.io/  URL for the server is  https://ci.vmware.run/
+To be able to change the integration between CI and GitHub, first become admin using the self serve portal.


Nit: self serve => self service

shaominchen · 2017-06-30T20:15:36Z

CI.md

+The CI system is based on https://drone.io/  URL for the server is  https://ci.vmware.run/
+To be able to change the integration between CI and GitHub, first become admin using the self serve portal.
+
+Behind the firewall there is a HW node running ESX that hosts 2 ESX VMs (v6.0u2 and v6.5). Each VM in turn contains a VSAN datastore as well as a VMFS datastore with 2 VMs per datastore (VMs are reused to form swarm cluster). CI testbed is also configured to have swarm cluster and there 3 VMs participating into the swarm cluster (2 VMs created on vmfs datastore and 1 VM created on vsan datastore).


Nit:

"Each VM in turn contains ... with 2 VMs per datastore" - we are using the same term "VM" in the same sentence to indicate ESX and Docker Host VM. I'd suggest to remove the former "VM" to avoid confusion, even though the ESX is indeed a VM.

there 3 VMs => there are 3 VMs.

participating into => participating in

shaominchen · 2017-06-30T21:18:54Z

CONTRIBUTING.md

-   * If you want to skip running CI test e.g. *Documentation change/Inline comment change* or when the change is in `WIP` or `PREVIEW` phase, add `[CI SKIP]` or `[SKIP CI]` to the PR title.
+   * Branch should be suffixed with github user id: `(branch name).(github user id)` Example: `mydevbranch.kerneltime`
+   * If you want to trigger CI test runs for pushing into a branch prefix the branch with `runci/` Example: `runci/mylatestchange.kerneltime`
+   * If you want to skip running CI test e.g. **Documentation change/Inline comment change** or when the change is in `WIP` or `PREVIEW` phase, add `[CI SKIP]` or `[SKIP CI]` to the PR title.
 * Each PR must be accompanied with unit/integration tests


Nit: Not your change, but I think we should say "Each PR should be accompanied with unit/integration tests whenever possible"

shaominchen · 2017-06-30T21:21:42Z

CONTRIBUTING.md

+
+- Create a branch, push changes and make sure tests do not break as reported
+  by the CI system.
+- When ready post a PR. This will trigger a full set of tests on ESX. After all


Nit: When ready post a PR. This will... =>When ready posting a PR, it will...

shaominchen · 2017-06-30T21:24:04Z

CONTRIBUTING.md

+**Note**: All 3 guest VMs can be reused to run swarm testcase by configuring swarm cluster (by making one node as master and registered others as worker nodes). It is not mandatory to create extra VM to configure swarm cluster but feel free if you would like to do so. Please make sure to set environment variables correctly as mentioned in section below.
+
+We require ESX 6.0 and later,
+and a Linux VM running  Docker 1.13+ enabled for  plain text TCP connection, i.e.


Nit: remove redundant spaces in this line

…ted page

vmwclabot added the cla-not-required label Jun 27, 2017

shuklanirdesh82 requested a review from msterin June 27, 2017 22:32

shuklanirdesh82 self-assigned this Jun 27, 2017

shuklanirdesh82 added the kind/documentation label Jun 27, 2017

shaominchen reviewed Jun 27, 2017

View reviewed changes

shaominchen approved these changes Jun 28, 2017

View reviewed changes

msterin suggested changes Jun 28, 2017

View reviewed changes

shuklanirdesh82 force-pushed the addingTestSetupDetail.shuklanirdesh82 branch 4 times, most recently from 5cb3288 to 8f81a34 Compare June 29, 2017 06:12

shuklanirdesh82 changed the title ~~Update CONTRIBUTING.md [SKIP CI]~~ Revamp CONTRIBUTING.md [SKIP CI] Jun 29, 2017

pshahzeb reviewed Jun 30, 2017

View reviewed changes

ashahi1 reviewed Jun 30, 2017

View reviewed changes

Contributing document revamp [SKIP CI]

a976e63

shuklanirdesh82 force-pushed the addingTestSetupDetail.shuklanirdesh82 branch 3 times, most recently from c55efbf to ceb84fd Compare June 30, 2017 18:04

pshahzeb approved these changes Jun 30, 2017

View reviewed changes

shaominchen approved these changes Jun 30, 2017

View reviewed changes

msterin approved these changes Jun 30, 2017

View reviewed changes

Addressing Govindan's comment-Separating out CI information to dedica…

c16de02

…ted page

shuklanirdesh82 force-pushed the addingTestSetupDetail.shuklanirdesh82 branch from ceb84fd to c16de02 Compare July 1, 2017 00:14

shuklanirdesh82 merged commit e37a514 into master Jul 1, 2017

shuklanirdesh82 deleted the addingTestSetupDetail.shuklanirdesh82 branch July 1, 2017 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp CONTRIBUTING.md [SKIP CI] #1495

Revamp CONTRIBUTING.md [SKIP CI] #1495

shuklanirdesh82 commented Jun 27, 2017 •

edited

shaominchen Jun 27, 2017

shaominchen Jun 27, 2017

shaominchen left a comment

msterin Jun 28, 2017

shuklanirdesh82 Jun 29, 2017

msterin Jun 28, 2017

shuklanirdesh82 Jun 29, 2017

msterin Jun 28, 2017

shaominchen Jun 29, 2017 •

edited

shaominchen Jun 29, 2017

msterin Jun 28, 2017

msterin Jun 28, 2017

shuklanirdesh82 Jun 29, 2017

pshahzeb left a comment

pshahzeb Jun 30, 2017

shuklanirdesh82 Jun 30, 2017

pshahzeb Jun 30, 2017

ashahi1 Jun 30, 2017

ashahi1 Jun 30, 2017

ashahi1 Jun 30, 2017

shuklanirdesh82 Jun 30, 2017

pshahzeb left a comment

shaominchen left a comment

shaominchen Jun 30, 2017

shuklanirdesh82 Jul 1, 2017

shaominchen Jun 30, 2017

shaominchen Jun 30, 2017

shaominchen Jun 30, 2017

shuklanirdesh82 Jul 1, 2017

shaominchen Jun 30, 2017

shaominchen Jun 30, 2017

shaominchen Jun 30, 2017

		@@ -146,9 +146,9 @@ of binaries in ./build directory.

		In order to test locally, you'd need a test setup. Local test setup automation is planned but but not done yet, so currently the environment has to be set up manually.


		## CI System

		We are using the CI system that has been up by the CNA folks (@casualjim, @frapposelli & @mhagen-vmware).

Revamp CONTRIBUTING.md [SKIP CI] #1495

Revamp CONTRIBUTING.md [SKIP CI] #1495

Conversation

shuklanirdesh82 commented Jun 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen Jun 29, 2017 • edited

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

pshahzeb 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

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

pshahzeb left a comment

Choose a reason for hiding this comment

shaominchen 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

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

shuklanirdesh82 commented Jun 27, 2017 •

edited

shaominchen Jun 29, 2017 •

edited