Fix permissions issues with Conda Environment on macOS and Linux #8402
Conversation
This reverts commit 7077811.
| } else { | ||
| return task.tool('sudo').line('conda'); | ||
| } | ||
| } |
There was a problem hiding this comment.
For cases where sudo is not installed, like for container image ubuntu:latest, can we run 'which sudo' to look for it, and if found use it, otherwise don't?
There was a problem hiding this comment.
Good suggestion. I would say a few things:
- Why doesn't
ubuntu:latesthavesudoinstalled? - I had originally hoped, instead of
sudo, to create envs in a directory that the non-root user has permissions for. Unfortunately, there's still some business with a temp file getting written to the /usr directory. (Whenever I've installed Miniconda it's ended up in~/miniconda, so maybe we should install it that way on the agent.) - Other tasks have already taken this approach, (Use Ruby Version comes to mind) and we have a general issue with needing to run tasks as root on Linux and macOS hosted agents now. I'd like to think through an approach that will work everywhere.
There was a problem hiding this comment.
wrt #1, I found this: tianon/docker-brew-ubuntu-core#48
I agree this could be smarter but sounds like there is an easy workaround: apt-get install sudo --yes. So I'd say it depends on how common we think this scenario will be.
| try { | ||
| await uut.condaEnvironment(parameters, Platform.Windows); | ||
| done(new Error('should not have succeeded')); |
There was a problem hiding this comment.
I was using both patterns before. I believe this way is better: failing tests in the old pattern (using done) would simply give the "timeout of 2000 ms has been reached, make sure you are calling done()" message.
| @@ -124,7 +124,7 @@ it('updates Conda if the user requests it', async function () { | |||
| assert(updateConda.calledOnceWithExactly('path-to-conda', Platform.Linux)); | |||
| }); | |||
|
|
|||
| it('fails if `conda` is not found', async function (done: MochaDone) { | |||
| it('fails if `conda` is not found', async function () { | |||
There was a problem hiding this comment.
async isn't needed here. I'm going to make a later change to clean up some stuff like this.
| * TODO move this to vsts-task-tool-lib | ||
| */ | ||
| export function prependPathSafe(toolPath: string) { | ||
| // TODO task-lib 2.4.0: `assertAgent` is not in mock-task |
There was a problem hiding this comment.
I've worked with @bryanmacfarlane's team on a plan for task-lib that will fix stuff like this. (Right now you can't mix custom mocks with setAnswers on mock-task.) I hope to get to that later this sprint.
| @@ -13,7 +13,7 @@ | |||
| "version": { | |||
| "Major": 1, | |||
| "Minor": 140, | |||
There was a problem hiding this comment.
It would be better to change the Minor version to the current sprint number.
There was a problem hiding this comment.
@jpricketMSFT I'm going to hotfix this to 140 and 141. We've decided in those cases to set the version according to the earliest sprint it will be backported to
| @@ -13,7 +13,7 @@ | |||
| "version": { | |||
| "Major": 1, | |||
| "Minor": 140, | |||
| "Patch": 0 | |||
| "Patch": 1 | |||
There was a problem hiding this comment.
Do we want to bump the minor to 142 now?
Hosted Ubuntu 16.04 does not run as root like the Docker container did.
Testing: