check: set CI=true so make check matches CI system

[grahamwhaley]

The CI system sets CI=true when running go-lint.sh, but
the Makefile does not, which results in many more linters
running from the Makefile, and failing.

Add a CI=true when running the make check (or more
accurately, make check-go-static, so we actually run
the same tests that the CI runs (and thus, they pass with
the current codebase)

Fixes: #646

Signed-off-by: Graham whaley graham.whaley@intel.com

[jodh-intel]

This might seem a bit "picky" and we could argue this either way... the change itself is fine. However, I'm actually not sure we want to do this as it sets a potentially dangerous precedent.

Certainly for Kata, if CI = true, the code can do basically whatever it likes (rm -rf $HOME, reformat drive, etc).

The vague idea is that if CI is not set, more checks and things will be run, and some/all of those additional checks may well fail. But if they pass, it would be possible for a dev to raise a new PR to add those new checks to the CI.

I'd rather we introduce a variable similar to KATA_DEV_MODE=true (https://github.com/kata-containers/tests#developer-mode) which, if set, will run in a similar fashion to the CI, but will not run any "dangerous" operations that might trash your system.

In summary, I'd prefer we change .ci/go-lint.sh so that the test looks for CI=true or DEV_MODE=true (or similar) and in either of those circumstances disables all linters before adding an explicit list of them.

[grahamwhaley]

I don't mind either way mostly. What I do want though is that a make check on a fresh checkout does the 'right thing', and does not throw >2k lines of errors, which is what it does for me today.

So, we could go and fix all those errors (eek!), or we make the default out of the box make check pass....

What I don't want to have to be doing is setting special env variables or adding CI=true to my make invocations - I just want this to work as expected, out of the box, for developers :-)

[sboeuf]

@grahamwhaley well I kind of agree with @jodh-intel on this one. If you put the CI=true inside the Makefile, then there is no point having this variable since it is gonna be set every time.
I don't have a strong opinion on this though, and if we decide to move forward and merge this, you should update your doc PR by removing references to CI=true.

[grahamwhaley]

So, how to move forwards then? There are a number of not necessarily compatible things here:

• I don't want to be setting any magic voodoo env vars, as a dev, when I do a checkout and build
• I don't really fancy trying to fix the 2k lines of lint errors we have today, unless, we say that is what needs to happen
• Understandably, @jodh-intel and @sboeuf don't want to have the user set CI=true, as that does sort of defeat the point of having that var

Does anybody have an objection if we hard wire the CI check in the linter script https://github.com/containers/virtcontainers/blob/master/.ci/go-lint.sh#L33-L35 to sort of the inverse - out of the box the CI and vanilla make do the same thing, but a dev can set a DEV_MODE env var that then enables all of the static checks.

Tell me, coz I'm curious.....:

• what do you do today - do you set CI=true in your env, or not run make check?
• does anybody really read that 2k log file to see if they added a new problem?

[jodh-intel]

Honestly, I currently set CI=true before running checks for vc. However, as mentioned, that could be dangerous in the general sense and as such we don't want to encourage it imho.

I think these are the main requirements:

• Have the CI run a fixed set of linters.
• Allow devs to run:
  • with the same setup as the CI.
  • with the full set of linters enabled.

Since setting CI=true could be dangerous on a dev system I think we need to:

• Change .ci/go-lint.sh so that it runs the subset of linters if CI=true or $condition.

The question comes down to what $condition should be set to:

• [ "$DEV_MODE" = true ]

  This is vaguely consistent with what we're doing with Kata. If this isn't set, all linters are run and the static checks will (probably) fail. But isn't that almost identical to the behaviour we have with the runtime if you try to build it with go build? The devs need to be educated to run make rather than the standard go build idiom. But that's fine as we've documented it: https://github.com/clearcontainers/runtime/blob/master/docs/developers-clear-containers-install.md#runtime.
  Once virtcontainers: Initial import kata-containers/runtime#41 lands, this all becomes academic since when built under Kata atleast, https://github.com/kata-containers/tests/blob/master/.ci/static-checks.sh will apply (which uses the KATA_DEV_MODE=true test).

• [ -z "$DEV_MODE" ]

  This is vaguely the opposite of what we're doing in Kata but would DTRT (TM) by default for new devs.

I personally don't think it's unreasonable to require a dev to set a variable or two as long as that setup is clearly documented, but I do think we need to discourage developers ever setting CI=true since although it would be fine in this circumstance, if they inadvertently ran some of the other CI scripts from other repos, the results could potentially be catastrophic.

[grahamwhaley]

OK, thanks for the input. I understand the 'opposite of what we currently do', but I do disagree that a dev should have to set some magic (even if it is documented magic) in order to build something out of the box. Also, I'm not sure we should just see this as 'a developer' that would be setting env vars - this is really just somebody who wants to build the project (although, sure, in the case of vc, as it is pretty much a go pkg/lib that is never really built on its own, that will almost 100% be developers).

Anyway, as you note, if we flip the use of DEV_MODE around, then it is a bit contrary to some of our other code bases. We don't currently use (afaict) DEV_MODE in this project, so how about we use another name. How about:

diff --git a/.ci/go-lint.sh b/.ci/go-lint.sh
index cdadaf4..0324ade 100755
--- a/.ci/go-lint.sh
+++ b/.ci/go-lint.sh
@@ -30,7 +30,7 @@ linter_args="--tests --vendor"
 # run locally, all linters should be run to allow the developer to review any
 # failures (and potentially decide whether we need to explicitly enable a new
 # linter in the CI).
-if [ "$CI" = true ]; then
+if [ "$CI" = true ] || [ -z "$FULL_STATIC_CHECKS" ] ; then
        linter_args+=" --disable-all"
 fi

That then both gives a clean out of the box build and test environment (well, apart from #645 which I may stare at as it bugs me every day at the moment), and also gives devs a way to turn on full linters if they desire. If we agree, I'll add it to the dev docs as well of course.

[jodh-intel]

FULL_STATIC_CHECKS wfm. What do others think?

[sboeuf]

@grahamwhaley @jodh-intel yes FULL_STATIC_CHECKS looks good and seems to be the right approach to me too.

[grahamwhaley]

Updated, code and commit message.

As per @jodh-intel note, we will need to co-ordinate merges of this #654 and #653 and now maybe #660 which might need to modify the dev docs again.

[sboeuf]

@grahamwhaley Need to be closed and moved to https://github.com/kata-containers/runtime/virtcontainers