Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Add cwd to the task plugin #116

Merged
merged 4 commits into from
Mar 22, 2019
Merged

Add cwd to the task plugin #116

merged 4 commits into from
Mar 22, 2019

Conversation

AndrienkoAleksandr
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr commented Mar 18, 2019

#116 (comment)

Signed-off-by: Oleksandr Andriienko oandriie@redhat.com

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@@ -53,6 +53,10 @@ export class CheTaskProvider {
resultTarget.machineName = await this.machinePicker.pick();
}

if (target && target.workingDir) {
resultTarget.workingDir = cheTaskDefinition.target.workingDir;
Copy link
Member

@RomanNikitenko RomanNikitenko Mar 21, 2019

Choose a reason for hiding this comment

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

You already have target here, so you can take workingDir from target directly, like resultTarget.workingDir = target.workingDir

@@ -59,13 +59,16 @@ export class CheTaskRunner {
throw new Error("Che task config must have 'target.machineName' property specified");
}

const workingDir = target.workingDir;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this const here? As I can see we don't use it in another places, so you can just pass target.workingDir. But up to you.

Copy link
Contributor

@olexii4 olexii4 Mar 21, 2019

Choose a reason for hiding this comment

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

so many devs, so many minds

const cwd = target.workingDir;
...
          tty: true,
          cwd
};

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

It would be helpful to mention workingDir property in the plugin's readme file as well.

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
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.

5 participants