这是indexloc提供的服务，不要输入任何密码
Skip to content

fix alert show in cli mode without accesskey (close #536) #588

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 9, 2018
Merged

fix alert show in cli mode without accesskey (close #536) #588

merged 4 commits into from
Oct 9, 2018

Conversation

@n1arash
Copy link
Contributor

@n1arash n1arash commented Sep 30, 2018

Description

now make sure that only if accessKey is exist show alert box

What component does this PR affect?

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Requires changes from other components? If yes, please mark the components:

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Related Issue

Solution and Design

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs update
  • Community content

Checklist:

  • I have read the contributing guide and my code conforms to the guidelines.
  • This change requires a change in the documentation.
  • I have updated the documentation accordingly.
  • I have added required tests.

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@hasura-bot
Copy link
Contributor

hasura-bot commented Sep 30, 2018

Review app available at: https://hge-ci-pull-588.herokuapp.com

@shahidhk shahidhk added the c/console Related to console label Oct 1, 2018
@karthikvt26
Copy link
Contributor

karthikvt26 commented Oct 1, 2018

@n1arash Thanks for the PR.

There are multiple problems associated with this.

  1. When Hasura console is loaded without the accessKey set by the CLI but server is configured with it, the console identifies this configuration to be login free but fails while trying to query data to render the baseRoute [Check-1] and while checking uptime [Check-2] of the CLI server.
  2. Now on the login page when you enter the accessKey, Check-1 succeeds but Check-2 fails as the hasura cli is still not configured with the accessKey.

I think for now it will be useful if we show valuable information in the alert box by decoding the code value in the response.

For example:

If the code returned by the migrate endpoint is access-denied then there is a possibility that access key is not set in CLI. We can show a message asking him to fix the issue.

Please let me know what you think.

@n1arash
Copy link
Contributor Author

n1arash commented Oct 2, 2018

do you mean when server is configured but cli is without key we should show the alert right?

@karthikvt26
Copy link
Contributor

karthikvt26 commented Oct 2, 2018

Yes instead of showing the response object itself we can provide a message on how to solve the problem?
‘Looks like CLI is not configured with the access key. Please configure and try again’?

@n1arash
Copy link
Contributor Author

n1arash commented Oct 2, 2018

@karthikvt26 yeah i agree with that i will commit changes ASAP

@n1arash
Copy link
Contributor Author

n1arash commented Oct 2, 2018

i added some changes so is this what we are looking for ?

@hasura-bot
Copy link
Contributor

hasura-bot commented Oct 2, 2018

Review app available at: https://hge-ci-pull-588.herokuapp.com

@shahidhk shahidhk requested a review from karthikvt26 October 5, 2018 05:20
karthikvt26
karthikvt26 requested changes Oct 5, 2018
View reviewed changes
@hasura-bot
Copy link
Contributor

hasura-bot commented Oct 8, 2018

Review app available at: https://hge-ci-pull-588.herokuapp.com

@karthikvt26 karthikvt26 added the s/ok-to-merge Status: This pull request can be merged to master label Oct 9, 2018
@shahidhk shahidhk merged commit dbdd79a into hasura:master Oct 9, 2018
@hasura-bot
Copy link
Contributor

hasura-bot commented Oct 9, 2018

Review app https://hge-ci-pull-588.herokuapp.com is deleted

@hasura-bot
Copy link
Contributor

hasura-bot commented Oct 9, 2018

Beep boop! 🤖

Whoa! 🎉 🎉 💃

GIF

Awesome work @n1arash! 💪 🏆 All of us at Hasura ❤️ what you did.

We would love to send you some swag! 👕 Please allow us to do so by filling this form.

If you have any questions, ask us on our Discord & Twitter!

Thanks again 🤗

@shahidhk shahidhk mentioned this pull request Oct 9, 2018
Closed
@shark-h
Copy link
Member

shark-h commented Nov 5, 2018

Hey @n1arash !
Can you please fill the form so that we can send you some swag? 😄

hasura-bot pushed a commit that referenced this pull request May 17, 2024
@danieljharvey @hasura-bot
Split out execute crate (#588)
2bb8be9 
<!-- Thank you for submitting this PR! :) -->

## Description

Following `metadata-resolve` and `schema` crates, this splits out
`execute`, the largest folder in `engine`. Undoubtedly this could be
split further.

Functional no-op.

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

Reviewers

@karthikvt26 karthikvt26 karthikvt26 approved these changes

Assignees

No one assigned

Labels

c/console Related to console s/ok-to-merge Status: This pull request can be merged to master

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@n1arash @CLAassistant @hasura-bot @karthikvt26 @shark-h @shahidhk