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

add default browser option to cli (close #3333) #3406

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
Closed
wants to merge 1 commit into from
Closed

add default browser option to cli (close #3333) #3406

wants to merge 1 commit into from

Conversation

@tibotiber
Copy link
Contributor

@tibotiber tibotiber commented Nov 23, 2019

Description

Hi!

This is the PR discussed in #3333 with @shahidhk. It adds the possibility to add HASURA_GRAPHQL_CLI_DEFAULT_BROWSER in your environment variables, which is picked up and used when running hasura console. If no value is provided, the default browser is used (doesn't change behaviour).

I have replaced Spin by normal logs as we may prefer to always see the browser being used.

Also I would have liked to add a note to the docs but it seems the CLI docs are auto generated by cobra so I'm not sure if I should touch these files.

Note that I am new to go so you might need to guide me a bit.

Affected components

  • CLI
  • Docs
  • Tests > is it needed for this changes?

Related Issues

#3333

Solution and Design

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No
  • Yes
    • Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

  • No
  • Yes
    • Does run_sql auto manages the new metadata through schema diffing?
      • Yes
      • Not required
    • Does run_sql auto manages the definitions of metadata on renaming?
      • Yes
      • Not required
    • Does export_metadata/replace_metadata supports the new metadata added?
      • Yes
      • Not required

GraphQL

  • No new GraphQL schema is generated
  • New GraphQL schema is being generated:
    • New types and typenames are correlated

Breaking changes

  • No Breaking changes

  • There are breaking changes:

    1. Metadata API

      Existing query types:

      • Modify args payload which is not backward compatible
      • Behavioural change of the API
      • Change in response JSON schema
      • Change in error code

    2. GraphQL API

      Schema Generation:

      • Change in any NamedType
      • Change in table field names

      Schema Resolve:-

      • Change in treatment of null value for any input fields

    3. Logging

      • Log JSON schema has changed
      • Log type names have changed

Steps to test and verify

To try it run HASURA_GRAPHQL_CLI_DEFAULT_BROWSER='Firefox Developer Edition' hasura console. Change to your browser of choice that is not your default browser.

Limitations, known bugs & workarounds

@tibotiber tibotiber requested a review from shahidhk as a code owner November 23, 2019 09:54
@hasura-bot
Copy link
Contributor

hasura-bot commented Nov 23, 2019

Beep boop! 🤖

Hey @tibotiber, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible.

Stay awesome! 😎

@CLAassistant
Copy link

CLAassistant commented Nov 23, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@tibotiber tibotiber mentioned this pull request Nov 23, 2019
Closed
@netlify
Copy link

netlify bot commented Nov 23, 2019

Deploy preview for hasura-docs ready!

Built with commit fdaf5e9

https://deploy-preview-3406--hasura-docs.netlify.com

@hasura-bot
Copy link
Contributor

hasura-bot commented Nov 23, 2019

Review app for commit fdaf5e9 deployed to Heroku: https://hge-ci-pull-3406.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3406-fdaf5e98

@marionschleifer marionschleifer added c/cli Related to CLI c/community Related to community content labels Nov 24, 2019
@marionschleifer marionschleifer added community-contrib 🙏🏽 and removed c/community Related to community content labels Dec 24, 2019
shahidhk
shahidhk requested changes Dec 26, 2019
View reviewed changes
Copy link
Member

@shahidhk shahidhk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @tibotiber and extremely sorry about the delay in reviewing this PR.

On giving a second thought at this, I think this configuration should just be a flag for the console command rather than an environment variable since it is only used on this command

Something like:

hasura console --browser chrome

Let me know what you think. 😄

@tibotiber
Copy link
Contributor Author

tibotiber commented Dec 27, 2019

Hi @shahidhk, a CLI flag sounds appropriate to me as well. I checked the source quickly and it seems easier to find examples than for environment variables. I'll try to have a go at it, probably after the new year.

Have a great time if you celebrate and speak soon.

@tibotiber
Copy link
Contributor Author

tibotiber commented Jan 6, 2020

Hi, I was gonna start reworking this but it seems @ashishra0 has already done this in #3601. Kindly update if his PR is good to go, then we can just close this and the related issue. I don't mind reworking this if it's still needed.

@shahidhk
Copy link
Member

shahidhk commented Jan 6, 2020

@tibotiber You can close this PR. If you could test out @ashishra0's PR and see if it works out for your use-cases, that would be great.

I'll credit you both in the merge commit.

@tibotiber
Copy link
Contributor Author

tibotiber commented Jan 6, 2020

@shahidhk I've test and looks good to me. Posted the same on the PR. Closing here. :)

@tibotiber tibotiber closed this Jan 6, 2020
@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 6, 2020

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

@shahidhk shahidhk reopened this Jan 7, 2020
@shahidhk shahidhk closed this Jan 7, 2020
@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 7, 2020

Beep boop! 🤖

Hey @tibotiber!

Sorry that your PR wasn’t merged.

Do take a look at any of the other open issues to see if you’d like to take something up! We’re around on Discord if you have any questions 😄

@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 7, 2020

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

@shahidhk
Copy link
Member

shahidhk commented Jan 7, 2020

@tibotiber Need your email address to add to the commit message.

@tibotiber
Copy link
Contributor Author

tibotiber commented Jan 8, 2020

Sent to your twitter DM.

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

Reviewers

@shahidhk shahidhk shahidhk requested changes

Assignees

No one assigned

Labels

c/cli Related to CLI community-contrib 🙏🏽

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tibotiber @hasura-bot @CLAassistant @shahidhk @marionschleifer