-
Notifications
You must be signed in to change notification settings - Fork 13
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
no local selection when docker is unavailable #662
Conversation
@@ -70,6 +84,7 @@ export default class AccountUtils { | |||
} | |||
} else { | |||
inquirer.registerPrompt('autocomplete', require('inquirer-autocomplete-prompt')); | |||
const can_run_local: boolean = await this.shouldListLocalAccounts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to specify the type here because the function return specifies.
const can_run_local = await this.shouldListLocalAccounts();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed specified type from can_run_local
@@ -49,7 +63,7 @@ export default class AccountUtils { | |||
const token_json = await app.auth.getPersistedTokenJSON(); | |||
if (!token_json || token_json.email === 'unknown') { | |||
console.log(chalk.yellow('In order to access remote accounts you can login by running `architect login`')); | |||
let account: Account; | |||
await this.shouldListLocalAccounts(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the output look like in this case? Won't it be kind of weird to show a warning log and then hard error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed hard-error, and replaced with warning and then exit if neither local or remote accounts are available.
@@ -70,6 +84,7 @@ export default class AccountUtils { | |||
} | |||
} else { | |||
inquirer.registerPrompt('autocomplete', require('inquirer-autocomplete-prompt')); | |||
const can_run_local = await this.shouldListLocalAccounts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if in this case we want to log a warning message to users. Example scenario here:
I think for the most part, if I don't have docker running, it's not that surprising to not have the local option and this warning message is just kind of superfluous. For a brand new user that maybe doesn't have docker running and would get confused, they probably don't have any remote accounts yet and would hit the hard-fail state above.
I'm leaning towards not logging anything if we're not hard-exiting, curious if anyone has thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed log statements for when local is not available.
# [1.23.0](v1.22.1...v1.23.0) (2022-08-15) ### Bug Fixes * **cli:** Switch from postinstall to prepare ([ffac972](ffac972)) * **dev:** Don't show ping access logs unless they fail ([33dc9b4](33dc9b4)) * **dev:** Make `architect dev` more robust ([#661](#661)) ([eda1cff](eda1cff)) * **exec:** Fix issue with commands run in no-tty but with stdin available exiting prematurely due to stdin closing ([#656](#656)) ([06ef0df](06ef0df)) * **exec:** no local selection when docker is unavailable ([#662](#662)) ([e3bf33b](e3bf33b)) * **register:** Allow register without build ([#660](#660)) ([033f261](033f261)) ### Features * **link:** 473 list linked components ([#658](#658)) ([b40c347](b40c347))
🎉 This PR is included in version 1.23.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Please see: https://gitlab.com/architect-io/architect-cli/-/issues/486