Repository: metal3-io/baremetal-operator
Pull request #577 (2020-07-06 20:13:14)

Update testing documentation with tool installation directions
Fixing a couple of variable declarations that were being
complained about by ``make lint``.

Edit: This was because of an outdated version of ``golint``. This patch will now focus on updating the testing documentation to add directions for installing latest testing tools.

Addresses: #575

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/577#" title="Author self-approved">demonCoder95</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **stbenjam**
You can assign the PR to them by writing `/assign @stbenjam` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["stbenjam"]} -->
metal3-io-bot: Hi @demonCoder95. Thanks for your PR.

I'm waiting for a [metal3-io](https://github.com/orgs/metal3-io/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/metal3-io/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

demonCoder95: /assign @stbenjam 
dhellmann: Which version of lint shows these errors? The one in our CI job doesn't report them, so I wonder if we're out of date?
demonCoder95: I'm not sure how to find the version of lint, I've tried various flags on the golint binary. I installed golint with APT which shows the following package version``Version: 0.0+git20161013.3390df4-1``

dhellmann: > I'm not sure how to find the version of lint, I've tried various flags on the golint binary. I installed golint with APT which shows the following package version`Version: 0.0+git20161013.3390df4-1`

I usually install the developer tools with `go get` so I can have the newer versions. In this case, `go get golang.org/x/lint/golint`.

The change you're making isn't wrong, I would just hate for us to all be using different linters. That's the point of running the lint job in CI.
demonCoder95: Ah! I see. Thanks for the pointer. I've just updated my linter and after running the linter again on master, there are no problems. 

I think it might be worthwhile to add this to the [testing documentation](https://github.com/metal3-io/baremetal-operator/blob/master/docs/testing.md), so that somebody just starting up might follow the same?
dhellmann: > Ah! I see. Thanks for the pointer. I've just updated my linter and after running the linter again on master, there are no problems.
> 
> I think it might be worthwhile to add this to the [testing documentation](https://github.com/metal3-io/baremetal-operator/blob/master/docs/testing.md), so that somebody just starting up might follow the same?

Updating the docs is a great idea. Do you want to do that instead of applying this PR? If so, you could mention the scripts in the `hack` directory as alternatives for installing the tools locally.
demonCoder95: That sounds great! I can revert these small changes within the same PR and add the doc update instead. The scripts in hack folder are very helpful, and it'll be great to have them indexed in the documentation!
dhellmann: > That sounds great! I can revert these small changes within the same PR and add the doc update instead. The scripts in hack folder are very helpful, and it'll be great to have them indexed in the documentation!

Perfect, thanks!
demonCoder95: @dhellmann had some issues with commit history. It's ready now. PTAL.
maelk: lgtm, but maybe the PR and the commit could be renamed to be a bit more descriptive of what the change actually does ? thank you
demonCoder95: Oh yes, I'll have to do that, since the reason the change started has changed, and it turned into a documentation patch. I'll update the descriptions accordingly. Thanks you @maelk 
maelk: thank you. Please amend your commit to rename it too.
demonCoder95: Done. Thanks for pointing it out @maelk 
maelk: Sure. And I was just thinking that maybe adding an explanation about the CONTAINER_RUNTIME variable could be nice, so that people that have docker and not podman installed can easily run the tests in containers too. Maybe together with an example ?
demonCoder95: That sounds good. Working on it.
demonCoder95: I just noticed that the IS_CONTAINER variable is inversed in logic. IS_CONTAINER=true actually ends up NOT running the tests in the container, and vice versa. Is it supposed to happen? or am I missing something? Just noticed this when making the example for docs.
demonCoder95: Waiting to put in the command output until I resolve the IS_CONTAINER variable issue. It seems to be it is incorrectly placed in all the scripts.
maelk: The IS_CONTAINER is correctly placed. There : https://github.com/metal3-io/baremetal-operator/blob/master/hack/golint.sh#L8 it checks that if the variable is not false, then it runs the make target. It is false by default, so it will by default go to run docker, that will run a container with that variable set to true, so then it will run the tests. So it is correct as it is now afaik.
demonCoder95: @maelk @dhellmann Just patched with the complete example and some improvements. Let me know what you think!
maelk: /test-integration
metal3-io-bot: @demonCoder95: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
markdownlint | 60a42a830648635d98664921296eb71f28999bda | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/577/markdownlint/1281095976219578373) | `/test markdownlint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: New changes are detected. LGTM label has been removed.
demonCoder95: /retest
metal3-io-bot: @demonCoder95: Cannot trigger testing until a trusted user reviews the PR and leaves an `/ok-to-test` message.

<details>

In response to [this](https://github.com/metal3-io/baremetal-operator/pull/577#issuecomment-655995010):

>/retest


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
demonCoder95: @maelk @dhellmann fixed the linter problems, should be good to go
maelk: /ok-to-test
metal3-io-bot: @demonCoder95: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
markdownlint | 664a7d93475d9f82c605f5ca0b68bc535a5649eb | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/577/markdownlint/1281176397070471173) | `/test markdownlint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @demonCoder95: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
markdownlint | 664a7d93475d9f82c605f5ca0b68bc535a5649eb | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/577/markdownlint/1281176397074665474) | `/test markdownlint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @demonCoder95: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
markdownlint | 53a1e534313e6224f9f00ea389b4bc516226047b | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/577/markdownlint/1281211000292380677) | `/test markdownlint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/cmd/make-bm-worker/templates/templates.go b/cmd/make-bm-worker/templates/templates.go
index 00d16847..bf51c466 100644
--- a/cmd/make-bm-worker/templates/templates.go
+++ b/cmd/make-bm-worker/templates/templates.go
@@ -22,7 +22,7 @@ kind: BareMetalHost
 metadata:
   name: {{ .Name }}
 spec:
-  online: false
+  online: true
 {{- if .HardwareProfile }}
   hardwareProfile: {{ .HardwareProfile }}
 {{- end }}
diff --git a/cmd/make-virt-host/main.go b/cmd/make-virt-host/main.go
index 41b86070..140382b0 100644
--- a/cmd/make-virt-host/main.go
+++ b/cmd/make-virt-host/main.go
@@ -68,7 +68,7 @@ kind: BareMetalHost
 metadata:
   name: {{ .Domain }}
 spec:
-  online: false
+  online: true
   bmc:
     address: libvirt://192.168.122.1:{{ .BMCPort }}/
     credentialsName: {{ .Domain }}-bmc-secret
diff --git a/docs/baremetalhost-states.md b/docs/baremetalhost-states.md
index 9cffc2b1..639875e9 100644
--- a/docs/baremetalhost-states.md
+++ b/docs/baremetalhost-states.md
@@ -11,10 +11,10 @@ Newly created hosts move immediately to Discovered or Registering. No
 host stays in the Created state while the operator is working
 properly.
 
-## Unmanaged
+## Discovered
 
-An Unmanaged host is missing both the BMC address and credentials
-secret name, and does not have any information to access the BMC
+A Discovered host is missing either the BMC address or credentials
+secret name, and does not have enough information to access the BMC
 for registration.
 
 ## Externally Provisioned
diff --git a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
index f419850b..945b7c8d 100644
--- a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
+++ b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
@@ -119,10 +119,6 @@ const (
 	// StateNone means the state is unknown
 	StateNone ProvisioningState = ""
 
-	// StateUnmanaged means there is insufficient information available to
-	// register the host
-	StateUnmanaged ProvisioningState = "unmanaged"
-
 	// StateRegistrationError means there was an error registering the
 	// host with the backend
 	StateRegistrationError ProvisioningState = "registration error"


Test patch:
diff --git a/cmd/make-bm-worker/templates/templates_test.go b/cmd/make-bm-worker/templates/templates_test.go
index c3784620..92a6aa32 100644
--- a/cmd/make-bm-worker/templates/templates_test.go
+++ b/cmd/make-bm-worker/templates/templates_test.go
@@ -51,7 +51,7 @@ kind: BareMetalHost
 metadata:
   name: hostname
 spec:
-  online: false
+  online: true
   hardwareProfile: hardware profile
   bmc:
     address: bmcAddress
@@ -86,7 +86,7 @@ kind: BareMetalHost
 metadata:
   name: hostname
 spec:
-  online: false
+  online: true
   bmc:
     address: bmcAddress
     credentialsName: hostname-bmc-secret
@@ -122,7 +122,7 @@ kind: BareMetalHost
 metadata:
   name: hostname
 spec:
-  online: false
+  online: true
   bmc:
     address: bmcAddress
     credentialsName: hostname-bmc-secret
diff --git a/docs/testing.md b/docs/testing.md
index eb2e68d6..632a015d 100644
--- a/docs/testing.md
+++ b/docs/testing.md
@@ -36,3 +36,66 @@ Run linters test before pushing your commit
 ```bash
 make lint
 ```
+
+## Using the Hack scripts
+
+The repository contains a ``hack`` directory which has some very useful scripts
+for setting up containerized testing environments. It also has Dockerfiles to
+allow you to generate images for your own use.
+
+All the scripts accept the ``CONTAINER_RUNTIME`` environment variable with default
+of ``podman``. This you can edit to ``docker`` as well.
+
+### Example of running ``golint`` in a Docker container
+
+1. Start by setting the environment variable appropriately
+
+    ```bash
+    export CONTAINER_RUNTIME=docker
+    ```
+
+2. From the operator parent dir, you can invoke the hack scripts
+
+    ```bash
+    ./hack/golint.sh
+    ```
+
+    **Note** It's important to be in the operator parent dir because it
+    contains the ``Makefile`` that is used in running the tests for the
+    operator. Otherwise, you'll see the following error
+
+    ```bash
+    sh: 0: Can't open /go/src/github.com/metal3-io/baremetal-operator/hack/golint.sh
+    ```
+
+3. Upon successful execution, you should see the following output. I already
+    had all the images available from a previous run, you might see the images
+    getting downloaded if you're running for the very first time.
+
+    ```bash
+    + IS_CONTAINER=false
+    + CONTAINER_RUNTIME=docker
+    + [ false != false ]
+    + docker run --rm --env IS_CONTAINER=TRUE --volume /home/noor/go/src/github.com/metal3-io/baremetal-operator:/go/src/github.com/metal3-io/baremetal-operator:ro,z\
+    --entrypoint sh --workdir /go/src/github.com/metal3-io/baremetal-operator\
+    quay.io/metal3-io/golint:latest /go/src/github.com/metal3-io/baremetal-operator/hack/golint.sh
+    + IS_CONTAINER=TRUE
+    + CONTAINER_RUNTIME=podman
+    + [ TRUE != false ]
+    + export XDG_CACHE_HOME=/tmp/.cache
+    + make lint
+    which golint 2>&1 >/dev/null || make OPATH/bin/golint
+    find ./pkg ./cmd -type f -name \*.go  |grep -v zz_ | xargs -L1 golint -set_exit_status
+    ```
+
+## Getting the latest tooling
+
+For testing without the hack scripts, make sure you install the latest
+development tools using ``go get``, e-g
+
+```bash
+go get golang.org/x/lint/golint
+```
+
+This ensures that the linters you have locally and the ones running in the CI
+are matched and helps avoid inconsistencies. Happy coding!