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

feat: basic open status command #74

Merged
merged 2 commits into from
Oct 18, 2022
Merged

feat: basic open status command #74

merged 2 commits into from
Oct 18, 2022

Conversation

loicsay
Copy link
Contributor

@loicsay loicsay commented Oct 18, 2022

No description provided.

@loicsay loicsay changed the title feat: badic open status command feat: basic open status command Oct 18, 2022
@loicsay loicsay self-assigned this Oct 18, 2022
@@ -23,6 +23,7 @@ var nameURLmap = map[string]string{
"docs": "https://algolia.com/doc/",
"cli-docs": "https://algolia.com/doc/tools/cli/get-started/overview/",
"cli-repo": "https://github.com/algolia/cli",
"status": "https://status.algolia.com/",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we could also open the dashboard on https://www.algolia.com/apps/<appID>/monitoring/status page.
Basically, if a profile / appID is currently configured / set as default, open the dashboard status page.
If no profile / no appID, open https://status.algolia.com/. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

The status page on the dashboard is more informative IMHO, as it clearly state which cluster you are and the status of the cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, It'd be even better when we'll handle authentication with the CLI, we could force login to the browser by passing a token (maybe?) so we're sure the user will never have to enter his login/password to see the https://www.algolia.com/apps/<appID>/monitoring/status page

Copy link
Contributor

@clemfromspace clemfromspace left a comment

Choose a reason for hiding this comment

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

One more comment :)

Comment on lines 127 to 130
url := nameURLmap[shortcut].Default
if applicationID != "" {
url = nameURLmap[shortcut].WithAppId
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have this logic at two different place now, I guess we could build the list of shortcuts before (using the AppID) and re-use it here and below?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I don't think the test at the line #105 is necessary anymore (adding the / before the appID)

Copy link
Contributor

@clemfromspace clemfromspace left a comment

Choose a reason for hiding this comment

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

👍

@loicsay loicsay merged commit e7d6d8b into main Oct 18, 2022
@loicsay loicsay deleted the feat/open-status branch October 18, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants