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

Added console command #247 #248

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@grallc
Contributor

grallc commented Aug 20, 2018

Added console command (#247) to open the application's console from CLI and removed dupllicated addonName arg

@hsablonniere

This comment has been minimized.

Member

hsablonniere commented Aug 20, 2018

@grallc Hey Corentin, thanks very much for you contribution 🎉

  • I have a few tiny remarks but it looks very good 😉
  • If you know how, I'd prefer you to amend your commit and force push. If you're not familiar enough with git, it's no problem, I'll handle it ;-)
  • FYI: The release of this code may take some time since we're in the middle of a big change on the releasing process...
@hsablonniere

Thanks again for your time ;-)

@@ -501,6 +500,13 @@ function run () {
options: [opts.alias],
}, open);
// CONSOLE COMMAND
const console = lazyRequireFunctionWithApi('../src/commands/console.js');

This comment has been minimized.

@hsablonniere

hsablonniere Aug 20, 2018

Member

I'm afraid this naming could confuse people with the already existing global variable console.

  • Could you rename this?
  • Maybe something like consoleModule or consoleFn
const Logger = require('../logger.js');
const OpenBrowser = require('../open-browser.js');
function console (api, params) {

This comment has been minimized.

@hsablonniere

hsablonniere Aug 20, 2018

Member

Same here 😄

I'm afraid this naming could confuse people with the already existing global variable console.

  • Could you rename this?
  • Maybe something like consoleModule or consoleFn
const s_open = AppConfig.getAppData(alias)
.flatMapLatest(({ app_id, org_id }) => {
Logger.println('Opening the console in your browser');
return OpenBrowser.openPage('https://console.clever-cloud.com/users/me/applications/' + app_id);

This comment has been minimized.

@hsablonniere

hsablonniere Aug 20, 2018

Member

I haven't tested but I think it won't work for apps in organizations.
We miss a test on the org_id to build the right URL in any case.

I also think we should use template strings for this:

const appUrl = `https://console.clever-cloud.com/users/me/applications/${app_id}`;

@grallc grallc force-pushed the grallc:console-command branch from 996989a to fc267e3 Aug 20, 2018

@grallc

This comment has been minimized.

Contributor

grallc commented Aug 20, 2018

Just pushed the amended commit.
Sorry for the organisation, I didn't know their existence... It's fixed now!
Thanks you for your tips ! 😄

@grallc grallc force-pushed the grallc:console-command branch 2 times, most recently from ae7b693 to 0e6d58c Aug 21, 2018

if (org_id) {
path = `organisations/${org_id}`;
}
return OpenBrowser.openPage(`https://console.clever-cloud.com/${path || 'users/me'}/applications/${app_id}`);

This comment has been minimized.

@hsablonniere

hsablonniere Aug 21, 2018

Member

What do you think of this?

const path = (org_id != null) ? `organisations/${org_id}` : 'users/me';
return OpenBrowser.openPage(`https://console.clever-cloud.com/${path}/applications/${app_id}`);

This comment has been minimized.

@grallc

grallc Aug 21, 2018

Contributor

Oh thanks!
I didn't know this syntax 😅

@hsablonniere

@grallc Thanks 👍 Don't worry, it's totally normal not to know this 😉

@grallc grallc force-pushed the grallc:console-command branch 3 times, most recently from 4d5163e to a98e3d8 Aug 21, 2018

@@ -13,7 +13,7 @@
"author": "Rodolphe Belouin <rodolphe.belouin@gmail.com>",
"license": "MIT",
"bin": {
"clever": "./bin/clever.js",
"clever-test": "./bin/clever.js",

This comment has been minimized.

@hsablonniere

This comment has been minimized.

@grallc

grallc Aug 21, 2018

Contributor

One more...
Really sorry for the spam :(

This comment has been minimized.

@hsablonniere

hsablonniere Aug 21, 2018

Member

This is not spam. This is iterative contribution 😉 which is always welcome.

@grallc grallc force-pushed the grallc:console-command branch from a98e3d8 to b92c9d1 Aug 21, 2018

@hsablonniere

This comment has been minimized.

Member

hsablonniere commented Aug 21, 2018

@grallc I'll test this in the afternoon and merge it if everything is OK.

hsablonniere added a commit that referenced this pull request Aug 21, 2018

@hsablonniere

This comment has been minimized.

Member

hsablonniere commented Aug 21, 2018

@grallc I just merged your commit manually (so I could remove the diff on the package-lock.json. Thanks again for you contribution!!!

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment