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

Using deploy -d with incorrect password #27

Closed
drmrbrewer opened this issue Aug 22, 2019 · 24 comments
Closed

Using deploy -d with incorrect password #27

drmrbrewer opened this issue Aug 22, 2019 · 24 comments
Labels
enhancement Something could be done better

Comments

@drmrbrewer
Copy link
Contributor

Prior to v2 I was able to run caprover deploy -d in the knowledge that if the CapRover machine password is no longer valid then I would be prompted for the current password and then the process would continue. Now, it just seems to bail out without a prompt:

Something bad happened: cannot deploy myapp at myserver.
Password is incorrect.

I prefer the previous behaviour... seems more user friendly just to be able to enter the password rather than having to go through the deploy process again without relying on defaults.

@githubsaturn
Copy link
Collaborator

I don't think this change was intentional. I'll look into it. You can always go back to the previous version via npm install -g caprover@1.2.0

@Hazzard17h was this an intended change?

@drmrbrewer
Copy link
Contributor Author

drmrbrewer commented Aug 22, 2019

Have reverted to 1.2.0 for now. Much better experience... get the following instead:

Your auth token is not valid anymore. Try to login again.
? Please enter your password for https://captain.myserver.mydomain.com

@drmrbrewer
Copy link
Contributor Author

Having upgraded to 2.0.2 to check that it addresses this issue (it does seem to), I can confirm that the issue reported here is back again (i.e. just bombing out if auth token is no longer valid rather than asking for password again). So I'm reverting to 1.2.0 again for now.

@githubsaturn
Copy link
Collaborator

@drmrbrewer
Correct. This wrong password issue hasn't been addressed yet.

@Hazzard17h
Copy link
Contributor

This was an intended change.
Basically, if we are re-deploying with --default, we want that all deploy configs are in the context, and we don't want user interaction.

If you think than must be done the authentication check (token check and ask password if expired), I can add it.

@drmrbrewer
Copy link
Contributor Author

Personally I'd find it very useful to have the behaviour as per v1... or at least have an option for it, i.e. to allow user interaction in the specific case where the auth token for accessing the caprover server is no longer valid. It enables me to do a quick caprover deploy -d (as part of a longer script that also performs other step like git commit etc), knowing that it will just ask me for the password if the current caprover server auth token is no longer valid. It would be a right royal pain to have to re-authenticate via another step, perhaps only 10 mins later when I come back to the terminal to discover that the deploy didn't complete. And I don't want to store my password in the script (coupled with the --pass option).

@Hazzard17h
Copy link
Contributor

IMHO, to use deploy in a script, the best option is -c config.json, to be sure that it doesn't break.

@drmrbrewer
Copy link
Contributor Author

IMHO, to use deploy in a script, the best option is -c config.json, to be sure that it doesn't break.

According to the docs, -c is not even an option.

@Hazzard17h
Copy link
Contributor

According to the docs, -c is not even an option.

Please, see updated docs related to the use of CLI, on the readme of this project: all commands accept -c configFile.

@drmrbrewer
Copy link
Contributor Author

Thanks. Still requires storing passwords in a file, though. Which the CLI v1 deploy -d method doesn't.

@Hazzard17h
Copy link
Contributor

Thanks. Still requires storing passwords in a file, though. Which the CLI v1 deploy -d method doesn't.

You could omit the password from config file, and CLI will ask for it if token is expired. But I don't figure why automatic deployment must involve user interaction...

@drmrbrewer
Copy link
Contributor Author

could omit the password from config file, and CLI will ask for it if token is expired

Why can't this logic apply to -d as well?

don't figure why automatic deployment must involve user interaction

In my case it's not automatic as such... I'm still sat at the terminal but I just run a single command (script) rather than several.

Something like:

./deploy 'some changes'

rather than:

git add -A .
git commit -m 'some changes'
git push origin master
caprover deploy -d

@Hazzard17h
Copy link
Contributor

Why can't this logic apply to -d as well?

Because -d is treated differently from other options, as said in the help: "use previously entered values for the current directory, no others options are considered".
If you think that must be done the authentication check (token check and ask password if expired), I can easily add it. But i don't think this is the intended functionality of -d.

In my case it's not automatic as such... I'm still sat at the terminal but I just run a single command (script) rather than several.

You can archive the same result updating the script to call caprover deploy expliciting deploy params, using -c config or directly needed options: -n caproverMachine -a app -b master.

@drmrbrewer
Copy link
Contributor Author

drmrbrewer commented Aug 28, 2019

You can archive the same result updating the script to call caprover deploy expliciting deploy params

Though if you don't want to store passwords in config files then you need to enter the password every time? With the v1 behaviour of the CLI you enter the password once and then only again when the auth token is no longer valid, which may be a looong time... which is very convenient.

@drmrbrewer
Copy link
Contributor Author

Just seems sensible and logical and helpful to be able to use defaults where available and if a piece of information is not available from defaults then ask for that missing of information. Like for -c. I'm not sure why there should be a distinction between -d and -c in this respect, regardless of what the docs currently say.

@Hazzard17h
Copy link
Contributor

Though if you don't want to store passwords in config files then you need to enter the password every time? With the v1 behaviour of the CLI you enter the password once and then only again when the auth token is no longer valid, which may be a looong time... which is very convenient.

No, if you store caproverName in the config, it try to reuse the auth token of that machine, and ask for a password only if it is no longer valid.

@drmrbrewer
Copy link
Contributor Author

OK. But what is the logic behind asking for it when using -c but not asking for it when using -d?

@Hazzard17h
Copy link
Contributor

The point is not -d vs -c, but how to handle -d without breaking things or have strange behaviors.
What if I use caprover deploy -d -u url? Or caprover deploy -d -i image, and previously used value is a branch and not an image?

As I said before, I could add the authentication check, but -d must be treated separately.

@drmrbrewer
Copy link
Contributor Author

how to handle -d without breaking things or have strange behaviors

Asking the user to enter the server password again IMHO is neither breaking things (it was like that in v1 so if anything it is not breaking things) and also it is not a strange behaviour. When using -d you're telling the CLI to use the previously-entered values, and the server name and password haven't changed since the last time you ran caprover deploy, so it is trying to use the previously-entered values as requested... the reason why in v1 it asks for the password again is (as I understand it) not because the password has changed (my server password has never changed) but because the auth token is no longer valid (e.g. because caprover restarted). The message in v1 is Your auth token is not valid anymore. Try to login again. So it seems reasonable and normal and not strange to be asked to authenticate again.

@githubsaturn
Copy link
Collaborator

Late to the party here. But here is my 2 cents:

  • Previously -d option, would use any previously entered/known values, if not known, or not valid, then it would ask the user.
  • With the new implementation, it assumes everything should be previously known / valid.

In general, I prefer the new behavior as it reduces the chance of wrongfully deploying over another app and other human errors. However, there is one exception to it, and that is the Auth Check.

I believe we should add back the auth check to the -d. The reason behind that is that the Auth Token can be expired via a simple server restart. The intended usage of -d is not for automation, it's for everyday development use. Without the auth check being integrated into the -d option, in case of a server restart, user has to enter everything again... quite frustrating...

So my suggestion is to keep things like they are, and just add the authentication check into the flow. Unless this creates a logical mess, I think this is the way to go.

@Hazzard17h / @drmrbrewer thoughts?

@drmrbrewer
Copy link
Contributor Author

My vote is for @githubsaturn as President of the Universe. Back more on topic, I think your summary is pretty much in line with my feelings expressed above (e.g. in the post just above yours) in that the auth check is different to asking for missing defaults as such. So apart from voting for @githubsaturn as POTU, my vote is to re-instate the auth check.

@Hazzard17h
Copy link
Contributor

It's fine for me, add authentication check is quite easy.

@githubsaturn githubsaturn added the enhancement Something could be done better label Aug 29, 2019
@Hazzard17h
Copy link
Contributor

See #33

@githubsaturn
Copy link
Collaborator

Fixed in #33 and released as 2.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something could be done better
Projects
None yet
Development

No branches or pull requests

3 participants