Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: cloud run command does not respect useLegacyWorkflow flag #4664

Merged
merged 1 commit into from
Jun 3, 2019

Conversation

rosen-vladimirov
Copy link
Contributor

@rosen-vladimirov rosen-vladimirov commented May 31, 2019

Currently when you run tns cloud run ... the command executes local prepare without taking into account the value of useLegacyWorkflow value in nsconfig.json file.
The problem is that cloud commands have specific -- options, which are set as dashedOptions on command level. During command execution the following actions happen:

  1. CLI starts its execution and via $injector constructs new instance of $options.
  2. In the constructor of $options we call this.setArgv(), which calls yargs package and constructs values based on the user input of -- options.
  3. $injector resolves $commandsService and constructs new object from it.
  4. In the constructor of CommandsService, we call $options.setupOptions and pass the current projectData. setupOptions takes into account the projectData and again calls this.setArgv(). After that it overwrites some of the values set by yargs based on the projectData.
  5. CommandsService starts execution of the command by resolving new instance of it and calling its own tryExecuteCommandAction method.
  6. tryExecuteCommandAction method internally calls this.$options.validateOptions by passing the command specific options(dashedOptions). The validateOptions method modifies the internal structure based on which yargs constructs the passed options and calls this.setArgv() again.

At this point we overwrite the internal values of $options based on the user's input and taking into account the command's dashedOptions. However, this way we've overwritten the values set by setupOptions in step 4 which are based on the current project dir.

To fix the behavior, make setupOptions private in $options and call it internally in the validateOptions method. So the new workflow is:

  1. CLI starts its execution and via $injector constructs new instance of $options.
  2. In the constructor of $options we call this.setArgv(), which calls yargs package and constructs values based on the user input of -- options.
  3. $injector resolves $commandsService and constructs new object from it.
  4. CommandsService starts execution of the command by resolving new instance of it and calling its own tryExecuteCommandAction method.
  5. tryExecuteCommandAction method internally calls this.$options.validateOptions by passing the command specific options(dashedOptions). The validateOptions method calls this.setupOptions and pass the current projectData and command specific options. After that it calls this.setArgv() and the internal structure that contains the passed options is overwritten based on the command specific options. After that the method overwrites some of the values based on the passed projectData.

PR Checklist

What is the current behavior?

What is the new behavior?

Fixes/Implements/Closes #[Issue Number].

@rosen-vladimirov rosen-vladimirov added this to the 5.4.1 milestone May 31, 2019
@rosen-vladimirov rosen-vladimirov self-assigned this May 31, 2019
@cla-bot cla-bot bot added the cla: yes label May 31, 2019
Currently when you run `tns cloud run ...` the command executes local prepare without taking into account the value of `useLegacyWorkflow` value in nsconfig.json file.
The problem is that cloud commands have specific `--` options, which are set as `dashedOptions` on command level. During command execution the following actions happen:
1. CLI starts its execution and via `$injector` constructs new instance of `$options`.
2. In the constructor of `$options` we call `this.setArgv()`, which calls `yargs` package and constructs values based on the user input of `--` options.
3. `$injector` resolves `$commandsService` and constructs new object from it.
4. In the constructor of `CommandsService`, we call `$options.setupOptions` and pass the current projectData. `setupOptions` takes into account the projectData and again calls `this.setArgv()`. After that it overwrites some of the values set by yargs based on the projectData.
5. `CommandsService` starts execution of the command by resolving new instance of it and calling its own `tryExecuteCommandAction` method.
6. `tryExecuteCommandAction` method internally calls `this.$options.validateOptions` by passing the command specific options(`dashedOptions`). The `validateOptions` method modifies the internal structure based on which yargs constructs the passed options and calls `this.setArgv()` again.

At this point we overwrite the internal values of `$options` based on the user's input and taking into account the command's `dashedOptions`. However, this way we've overwritten the values set by `setupOptions` in step 4 which are based on the current project dir.

To fix the behavior, make `setupOptions` private in `$options` and call it internally in the `validateOptions` method. So the new workflow is:
1. CLI starts its execution and via `$injector` constructs new instance of `$options`.
2. In the constructor of `$options` we call `this.setArgv()`, which calls `yargs` package and constructs values based on the user input of `--` options.
3. `$injector` resolves `$commandsService` and constructs new object from it.
4. `CommandsService` starts execution of the command by resolving new instance of it and calling its own `tryExecuteCommandAction` method.
5. `tryExecuteCommandAction` method internally calls `this.$options.validateOptions` by passing the command specific options(`dashedOptions`). The `validateOptions` method calls `this.setupOptions` and pass the current projectData and command specific options. After that it calls `this.setArgv()` and the internal structure that contains the passed options is overwritten based on the command specific options. After that the method overwrites some of the values based on the passed projectData.
@rosen-vladimirov rosen-vladimirov force-pushed the vladimirov/fix-cloud-commands branch from 188e72d to 80e6ca8 Compare May 31, 2019 15:17
@rosen-vladimirov
Copy link
Contributor Author

test cli-smoke

Copy link
Contributor

@KristianDD KristianDD left a comment

Choose a reason for hiding this comment

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

Just log a separate issue to iterate over improving the options handling.

@rosen-vladimirov
Copy link
Contributor Author

test cli-smoke nativescript-dev-typescript#latest

@dtopuzov
Copy link
Contributor

dtopuzov commented Jun 2, 2019

test cli-smoke nativescript-dev-typescript#latest tns-core-modules#5.4.0

1 similar comment
@rosen-vladimirov
Copy link
Contributor Author

test cli-smoke nativescript-dev-typescript#latest tns-core-modules#5.4.0

@rosen-vladimirov rosen-vladimirov merged commit aa89134 into release Jun 3, 2019
@rosen-vladimirov rosen-vladimirov deleted the vladimirov/fix-cloud-commands branch June 3, 2019 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants