Enhancements #16

Hazzard17h · 2019-05-14T16:18:16Z

To enhance automation capabilities, I standardized parameters retrieval from config, options and questions, for login and logout commands. If the solution fit, we can enlarge it to others commands.

… applied to login

githubsaturn · 2019-05-15T16:02:38Z

Thanks @Hazzard17h !

It's a rather large PR. I won't be able to get to in next 2 weeks as my schedule is pretty tight. But I had a brief look at it and here is my thoughts:

Using a config file was a choice by design. The goal is provide the gear for automation, at the same time, not shooting ourselves in the foot by making the maintenance complex. Using the explicit approach may look simpler, however, it causes confusion for general audience of CapRover (you'd be surprised!)
So instead of breaking down the config parameters, let's create a helper function that "creates" the config file using the explicit parameters, then automation can simply use that command in conjunction with --configFile and take best of both worlds!! Otherwise, any config file schema update requires multiple places to be updated in the code (higher maintenance cost)
The stateful commands should never be automated. Commands such as login and logout are stateful and are meant to be used by real humans. Action commands, such as deploy, can be used in automation and they do not require a logged in user.
I like some of the refactoring that you did! Cleanups are the best!

Last but not least, awesome work! If you're interested in automation improvement, there is a ticket that's sitting there and it's on my high priority list for a while now:
#12 (comment)

Are you interested in taking that over?

Hazzard17h · 2019-05-16T08:42:31Z

My end goal is to implement that generic command that you mention in #12, but I first started with some enhancements to become familiar with the code.

About config file, explicit command options and command line question, my thought is: if --configFile is specified as option, read that file for parameters, than overwrite any parameter passed as explicit command option, at last ask through command line question any missing (and required) parameter.
That approach requires that names for command options coincide with questions' names, but also allows to know config file schema by looking command help.
Any parameter update requires two places to be updated in the code: the command option and the command line question. I'm thinking to generate both from an object that define all useful properties to increase maintainability and have a unique source of truth.
About stateful commands automation: login already has automation through config file. I believe that could be useful to login (maybe even on multiple machines) at the beginning of automation and do multiple commands using logged-in machine name parameter, instead of login for any command.

…erUrl and defaulted to https

Hazzard17h · 2019-06-18T13:32:45Z

@githubsaturn if you have a chance to review the code, now the enhancements are completed with standardized behaviors for all commands, fully backward compatibility with the current functionalities, and also support environment variables.

Next I'll start to implement some sort of generic command.

githubsaturn · 2019-06-20T05:41:05Z

@Hazzard17h

Apologies for the delay. I'm still struggling to find some time - by the end of June my schedule will be more flexible. But this is definitely at the top of my list. Changes look okay. I just need to do a deeper dive into the code and run some tests to make sure that the existing behavior isn't broken.

Have you tested all the functionalities?

PS: If you're planning on extending this work and adding more functionality, please create another branch. This branch is already very crowded and it's hard to code review added functionalities if they are brought to the mix.

Thanks,
Kasra

Hazzard17h · 2019-06-20T09:11:04Z

Not worry.

I tested all the functionalities, but surely another review is recommended.

githubsaturn · 2019-07-29T20:31:26Z

I've noticed there are many duplication in the code. For example, caprover deploy --help is now very crowded - unnecessary aliases for host, password and etc. We can't merge this PR as it is right now.

When I first saw the PR, I thought of it as some reordering of method and etc. But I am seeing more changes in it. I'm going to park this PR as is. There are definitely some good changes and cleanups such as the inheritance that you added for Command. But due to a larger scope of changes we can't merge this as is. We can either, publish this (and effectively the one with generic API) as a more configurable/advanced client, or just start cutting down on changes and go step by step.

There are many good changes in this PR that I wanted to keep if we can reduce the scope of changes in this PR. Specifically, we want to keep the changes to the public API to zero during a big refactoring like this.

Hazzard17h · 2019-07-30T09:26:43Z

Indeed the changes to the public API are zero, even though the big refactoring of the CLI.
Also all command options and config of the previous version are still compatible, for this purpose I introduced the aliases, and applied them to the deploy command.
Moreover I think that all open issues are handled by this PR, so following a careful review and test by you, it could be surely merged.

githubsaturn · 2019-07-30T11:45:35Z

@Hazzard17h

I found some discrepencies, for example, machineName is renamed to caproverName. Also, other commands have many redundant aliases now. For example, caprover deploy takes --appName and --caproverApp. Can you elaborate why these aliases are added? Maybe what you're saying is that the scope of changes is limited?

Hazzard17h · 2019-07-30T13:28:14Z

All commands now are standardized:

common options have same names (--configFile, --caproverUrl, --caproverPassword, --caproverName, ...), I just selected the names by the most common already in use, and added aliases to those I changed to keep backward compatibility;
can take config file via -c option, parameters names in the file respect the command options;
can take environment variables;
needed values not provided are asked on the command line.

With this new flow, one user could create a common config file with every value needed, from server setup, to app deploy. And already in use deploy scripts will continue to work, also updating the CLI.

githubsaturn · 2019-07-31T21:55:50Z

I see. In that case, I'll go over this again. I might have some more questions which I will post here. Thanks

githubsaturn · 2019-07-29T19:51:54Z

package.json

-    "caprover-login": "./built/commands/login.js",
-    "caprover-logout": "./built/commands/logout.js",
-    "caprover-serversetup": "./built/commands/serversetup.js"
+    "caprover": "./built/commands/caprover.js"


Was there a particular reason that these were removed? They were added to fix an issue with yarn.

a1ff473

The only entry point is caprover cli; sub commands need to be built by the main cli and they can't be called directly. I don't think it could be a problem.

That's what I expected as well, but it turned out that, if installed using yarn, it becomes a problem. See this:
caprover/caprover#338 (comment)

githubsaturn · 2019-07-29T19:53:29Z

readme.md

-  "emailForHttps": "[email protected]",
-  "currentPassword": "captain42"
+  "certificateEmail": "[email protected]",
+  "caproverName": "my-machine-123-123-123-123",


Let's avoid renaming these properties (all of them). This renaming would break the existing automation pipeline.

This is only the readme with new standardized names for new users.
As you can see in the options definition for serversetup command (and other too), many options have aliases to preserve compatibility with previous version, for example here.
If I've forgotten something we could surely add more aliases.

githubsaturn · 2019-07-29T19:55:52Z

readme.md

 {
-  "hasRootHttps": "true",
-  "caproverPassword": "captain42",
  "caproverUrl": "captain.root.domain.com",


Not all installations have SSL certs. Some people install CapRover on local domain names. Let's bring the hasRootHttps back. Or better than that, we can allow the "https://" or "http" be present in the URL, and we can use the implicit value

See cleanAdminDomainUrl: we allow "http" and "https" (default if not specified), and also support hasRootHttps in login config file for backward compatibility, as you can see here.

githubsaturn · 2019-07-29T19:57:39Z

readme.md

-You can also deploy directly with one command:
-```bash
-caprover deploy -h https://captain.root.domain.com -a app-name -p password -b branchName
+caprover deploy -n machine-name -a app-name -b branchName


Why the deploy command has changed its signature? -h and -p are gone. The main purpose of this command was automated deploys without having to be logged in.

As I said before, I changed the readme with the new standardized options, but all of them previously present are also supported.
As you can see in deploy command help, you can use deploy -u url -p password to deploy to a not logged in server.

githubsaturn · 2019-07-29T20:02:41Z

src/commands/Command.ts

+export interface IOption extends inquirer.Question, IOptionAlias {
+    name: string,
+    aliases?: IOptionAlias[]
+    tap?: (param?: IParam) => void


What's tap? Maybe a more descriptive name?

Tap is a function called after the value of the option is acquired, to perform side effect on that option. Name is inspired from the tap operator of RxJs Obsevable...

Got it. I don't think tap is the best name, it makes sense in Rx world due to "streams" and "pipes". I think we should rename it to onOptionSelected or something similar... I can make this change later, it's not a public API - so all go for now.

githubsaturn · 2019-07-29T20:05:48Z

src/commands/login.ts

-        return true
-    }
+const K = Utils.extendCommonKeys({
+    https: 'hasRootHttps'


Why don't we move everything to the main common keys object. That way, we can make sure that there is no duplication of literal strings anywhere.

The idea is to have in the constants only common options keys, not that ones that are only in one command.

githubsaturn · 2019-07-29T20:07:28Z

src/commands/serversetup.ts

+        {
+            name: K.ip,
+            char: 'i',
+            env: 'CAPROVER_IP',


Let's move all env vars to an EnvKeys constant for better documentation.

We can create in constants file the base option definition for all common options, to have consistency with name, char, env, across all commands.

githubsaturn · 2019-07-29T20:11:30Z

src/utils/Utils.ts

    },
+
+    isValidEmail(email: string): boolean {


Let's add some unit tests for these utils functions

Could be done, but no unit test was done before, so I don't added anyone now.

githubsaturn · 2019-07-29T20:17:01Z

src/commands/list.ts

@@ -1,29 +1,22 @@
-#!/usr/bin/env node


These shouldn't be removed. See my comment in package.json

As I said before, the only entry point is caprover cli, so we don't need this.

githubsaturn · 2019-08-01T15:50:13Z

src/utils/CliHelper.ts

+                await CliApiManager.get(machine).getAllApps() // Get data with stored token
+            } catch (e) { // Error getting data: token expired
+                StdOutUtil.printWarning(`Your auth token for ${StdOutUtil.getColoredMachine(machine)} is not valid anymore, try to login again...`)
+                StorageHelper.get().removeMachine(machineName) // Remove saved credentials


Why are we removing the machine entirely? This was not the case before. In this case, if the user fails to login again, the machine is completely lost and they have to re-enter the cred.

I'm going to fix this one.

githubsaturn · 2019-08-05T22:04:52Z

Great, thanks @Hazzard17h

Lots of my comments and confusion was caused by aliases, so we should be good. Some other modifications are still to be made, but they aren't public API, so we'll leave them for now. Cheers.

githubsaturn · 2019-08-13T19:26:02Z

src/commands/Command.ts

+        })
+
+        cmd.action(async (cmdLineOptions: ICommandLineOptions) => {
+            cmdLineOptions = await this.preAction(cmdLineOptions)


@Hazzard17h

Unfortunately, I only found out about this after I release the new version.

This, cmdLineOptions: ICommandLineOptions, seems to be wrong. The argument to the action method is not an array of options. It's the whole Command object. I believe what you wanted to use is cmdLineOptions.options which translate to an object that has simple (key:string) => (value: string | boolean) structure.

Would you be able to fix the issue? Otherwise, let me know and I'll look into it. From what I can see, it's only being used to override the actual parameters with the new aliases that you've added. Right?

PS: Another issue in the same pipeline is that running an unknown command causes a problem. For example caprover deploy random

As you can see here, the action of a program sub-command takes a function which has as many arguments as the positional parameters of the sub-command, and at least the entire sub-command object, with the options added as keys.
So, the code works if called normally, but as you had noted, if we pass a positional argument to a sub-command (e.g. caprover deploy asd), it is broken.
I'm fixing it, by trowing an error, if user provided a position parameter.

Hazzard17h added 3 commits May 14, 2019 11:13

Improved login

3f8a406

Standardized parameters retrieval from config, options and questions:…

a947475

… applied to login

Standardized logout

399f2d1

Hazzard17h added 15 commits May 16, 2019 16:02

Removed --hasRootHttps param from login, it is deducted from --caprov…

279de4f

…erUrl and defaulted to https

Updated utils

abf42a8

Added Command base class in plase of CommandHelper

4393d89

Updated validations and CliHelper to manage new commands structure

43d00cb

Updated login command

f37790c

Updated list command

ed275ad

Updated logout command

7625ea3

Updated deploy command

dd99e04

Updated cli to new commands structure

52180a6

Fixed Command to preserve options management order

53ebf3a

Fixed commands for new Command management

c451478

Updated serversetup command

fdf8c8d

Updated default config file option

a1d427e

Added support to environment variables

2ff3219

Added support for options aliases

e110225

Updated readme

1704a20

Hazzard17h mentioned this pull request Jun 25, 2019

Generic api #19

Closed

githubsaturn requested changes Aug 1, 2019

View reviewed changes

Fix retry login on token expired

cb0c706

githubsaturn merged commit bdfc602 into caprover:master Aug 5, 2019

Hazzard17h deleted the enhancements branch August 6, 2019 08:33

githubsaturn reviewed Aug 13, 2019

View reviewed changes

This was referenced Sep 2, 2019

Better error message for bad user input, maybe? #14

Open

Incorrect password throws error #13

Open

Enhancements #16

Enhancements #16

Conversation

Hazzard17h commented May 14, 2019

githubsaturn commented May 15, 2019

Hazzard17h commented May 16, 2019

Hazzard17h commented Jun 18, 2019

githubsaturn commented Jun 20, 2019

Hazzard17h commented Jun 20, 2019

githubsaturn commented Jul 29, 2019 • edited Loading

Hazzard17h commented Jul 30, 2019

githubsaturn commented Jul 30, 2019

Hazzard17h commented Jul 30, 2019

githubsaturn commented Jul 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

githubsaturn commented Aug 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

githubsaturn commented Jul 29, 2019 •

edited

Loading