minishift / minishift-addons

A repository for the community to exchange Minishift add-ons
Apache License 2.0
71 stars 86 forks source link

[che] Add test with running workspace with `anyuid` addon enabled #132

Closed ibuziuk closed 6 years ago

ibuziuk commented 6 years ago

Need to add test that would cover Unable to start Che workspaces on OCP with 'anyuid' enabled [1] issue.

Basically, it looks like that starting from 3.7 version of origin pvs are owned by root & root group. It works with arbitary users since it is part of the root group, and the user created in the workspace image (which is used in workspace pod when anyuid is enabled) does not belong to this group and pvc are not writable for this user. The root access for pvc is probably related to the following origin issue [2]. Workaround [3] on che side was adding user to the root group.

The test should do the following

[1] https://issues.jboss.org/browse/CDK-305 [2] https://github.com/openshift/origin/issues/14076 [3] https://github.com/eclipse/che-dockerfiles/pull/198

praveenkumar commented 6 years ago

@agajdosi Can't we just extend https://github.com/minishift/minishift-addons/tree/master/test/integration here for this use case?

agajdosi commented 6 years ago

@praveenkumar I was thinking the same yesterday, if we agree that we are ok with testing Che addon with anyuid enabled (or maybe all default add-ons enabled) then I can just update current feature file (test). WDYT @ibuziuk

ibuziuk commented 6 years ago

@agajdosi I really think that the main test scenarios should be with anyuid disabled. We spent a lot of time to make che running as an arbitrary user and this should be tested properly since this config is used for downstream projects like openshift.io. What would be great to have is the same set of tests running with and without anyuid (or at very least having one test with anyuid enabled). This would improve coverage dramatically IMO.

agajdosi commented 6 years ago

@ibuziuk Thanks for sharing this information, reasons for having 2 tests are now pretty clear. Will add the case with anyuid enabled as separate test.

praveenkumar commented 6 years ago

Yes we should have both test case with/without anyuid.

LalatenduMohanty commented 6 years ago

@ibuziuk @agajdosi This test should be part of minishift repository, so that we can test each PR or at least nightly with this test.

ibuziuk commented 6 years ago

@LalatenduMohanty do you mean minishift-addons ? minishift repo does not seem to contain che by default - https://github.com/minishift/minishift/tree/master/addons

LalatenduMohanty commented 6 years ago

do you mean minishift-addons ? minishift repo does not seem to contain che by default - https://github.com/minishift/minishift/tree/master/addons

Good point. The reason we have kept Che in minishift-addons because it was not stable at that time. may be it is time to re-evaluate it. Personally I want to have the test in the minishift repo even if Che is not part of the repository.

ibuziuk commented 6 years ago

I would be really happy if che would be a part of the default set of addons!

Personally I want to have the test in the minishift repo even if Che is not part of the repository.

Well, that might be a bit misleading IMO if addon is not part of the defaults. It would be great if we could add both addon and tests to the minishift though.

The reason we have kept Che in minishift-addons because it was not stable at that time. may be it is time to re-evaluate it.

This should be quite stable at this point of time and well documented on both che [1] and minishift [2] sides

[1] https://www.eclipse.org/che/docs/openshift-single-user.html#minishift-addon [2] https://github.com/minishift/minishift-addons/blob/master/add-ons/che/README.md

agajdosi commented 6 years ago

Personally I want to have the test in the minishift repo even if Che is not part of the repository.

@LalatenduMohanty Still the code should live at m-addons (if Che addon remains here) and we should only import the package. Right now I am however not sure (suspicious of circular dependency) if that would be possible (need to check for possible solutions).

What bothers me more is that AFAIK there is no way to use dep to handle the feature files, so we would need to take care of updating duplicate files at 2 locations.

This test should be part of minishift repository, so that we can test each PR or at least nightly with this test.

Because of stuff above^^ we should maybe consider not having Che addon tests as part of the repository, but having minishift-addons tests as part of PR/nightly/release checks. Once tests present in minishift repository are passed then we can just clone minishift-addons repo and run whatever we want from there.

Or we just add Che addon itself and its tests to minishift. If that would make @ibuziuk really happy, we should consider :wink:

agajdosi commented 6 years ago

Hi all, I am closing this issue as we moved Che to Minishift repository. Corresponding issue: https://github.com/minishift/minishift/issues/2597.