Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

CI: Disable root tests in dev mode #30

Merged

Conversation

jodh-intel
Copy link
Contributor

Don't run the unit tests as root in developer mode (when
KATA_DEV_MODE) is set.

Fixes #29.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

@jodh-intel
Copy link
Contributor Author

Hi @chavafg, @sameo, @sboeuf, @grahamwhaley, @bergwolf - what do you think?

If we decide that developers really do want to run tests as root locally, my view is that they should be required to set an extra variable (KATA_DEV_MODE_RUN_AS_ROOT=true or something).

But it should be considered very unsafe for a developer to set CI in their environment since all scripts should assume if that variable is set that they are running on a real CI system (so can do whatever they like).

@chavafg
Copy link
Contributor

chavafg commented Jan 19, 2018

I agree that developers should not run the tests as root.
Could it be possible that a developer does not know about KATA_DEV_MODE flag and sets CI=TRUE? In this case, will he run tests as root without noticing?
If so, we would need to document how the a developer should run the tests.

@jodh-intel
Copy link
Contributor Author

@chavafg - I agree. We need a README. I'm happy to write one (either on this PR or another).

@jodh-intel
Copy link
Contributor Author

Hi @chavafg - I'm going to add a README on #31 (as the content in master already mentions KATA_DEV_MODE).

@jodh-intel
Copy link
Contributor Author

See #32.

@chavafg
Copy link
Contributor

chavafg commented Jan 22, 2018

lgtm, thanks for the README

Approved with PullApprove

@jodh-intel
Copy link
Contributor Author

Could we get one more review on this PR please?

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the README will now be a necessity - the combinatorics are getting complex.
One minor nit, but, given this is what we want to do then:
lgtm

.ci/go-test.sh Outdated
users="current root"
if [ -n "$KATA_DEV_MODE" ]; then
warn "Dangerous to set CI with KATA_DEV_MODE set"
warn "Not also running tests as root"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rephrase this line - it doesn't quite make sense in my head....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - reworded for improved clarity.

Don't run the unit tests as root in developer mode (when
`KATA_DEV_MODE`) is set.

Fixes kata-containers#29.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel jodh-intel force-pushed the disable-root-tests-in-dev-mode branch from 292997a to e74ded9 Compare January 23, 2018 10:32
@jodh-intel jodh-intel merged commit c730ff0 into kata-containers:master Jan 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants