Skip to content

Allow setting rest and graphQL flags and GraphQL path from CLI#1309

Merged
abhishekkumams merged 33 commits intomainfrom
dev/abhishekkuma/allow_setting_rest_and_graphql_runtime_flags
Mar 20, 2023
Merged

Allow setting rest and graphQL flags and GraphQL path from CLI#1309
abhishekkumams merged 33 commits intomainfrom
dev/abhishekkuma/allow_setting_rest_and_graphql_runtime_flags

Conversation

@abhishekkumams
Copy link
Contributor

@abhishekkumams abhishekkumams commented Mar 6, 2023

Why make this change?

What is this change?

  • adding 3 new options to CLI init command:
    • --graphql.path : To provide custom graphql path
    • --rest.disabled: To disable rest endpoints globally
    • --graphql.disabled: To disable graphql endpoints globally

Why did i use --rest.disabled/--graphql.disabled instead of --rest.enabled/--graphql.enabled ?

  • the boolean options do not accept values. so, we cannot do --rest.enabled false. Just using this flag means true.
  • By default REST/GraphQL points are enabled, hence it's more convinient to use --rest.disabled/--graphql.disabled only when we want to disable it. reduces the length of command.

How was this tested?

  • Integration Tests
  • Unit Tests

Sample Request(s)

dab init --database-type mssql

"runtime": {
    "rest": {
      "enabled": true,
      "path": "/api"
    },
    "graphql": {
      "allow-introspection": true,
      "enabled": true,
      "path": "/graphql"
    },
...
  }

dab init --database-type mssql --rest.disabled --graphql.disabled

"runtime": {
    "rest": {
      "enabled": false,
      "path": "/api"
    },
    "graphql": {
      "allow-introspection": true,
      "enabled": false,
      "path": "/graphql"
    },
...
  }

dab init --database-type mssql --rest.disabled --graphql.path /mygraphql

"runtime": {
    "rest": {
      "enabled": false,
      "path": "/api"
    },
    "graphql": {
      "allow-introspection": true,
      "enabled": true,
      "path": "/mygraphql"
    },
...
  }

image

@ayush3797
Copy link
Contributor

ayush3797 commented Mar 7, 2023

We can instead use string instead of bool to represent these newly added boolean options, and then we can have the privilege of setting them via CLI as well. A similar thing would have to be done for the existing set-session-context boolean option as well.

reduces the length of command.

In my opinion, we should remain consistent with following the pattern of <--option-name> <--option-value> in CLI. To me as a user, it would be a bit confusing.

@abhishekkumams
Copy link
Contributor Author

abhishekkumams commented Mar 7, 2023

We can instead use string instead of bool to represent these newly added boolean options, and then we can have the privilege of setting them via CLI as well. A similar thing would have to be done for the existing set-session-context boolean option as well.

reduces the length of command.

In my opinion, we should remain consistent with following the pattern of <--option-name> <--option-value> in CLI. To me as a user, it would be a bit confusing.

That is definitely a way but it's not convenient in my opinion. even the --set-session-context should use bool instead of string. So, they can simply set it using the option directly, as set itself appears to be setting it to true.

@abhishekkumams abhishekkumams self-assigned this Mar 9, 2023
@abhishekkumams abhishekkumams marked this pull request as ready for review March 14, 2023 07:13
@Aniruddh25
Copy link
Collaborator

At the moment, SWA routes all GraphQL requests to /graphql. When providing the ability to customize this path, the SWA team needs to be made aware of this feature (they might already be aware or the feature request might have originated from them in the first place), so that they route it to the custom path and not the default path. Just want to bring to attention that we need to be in sync with them so that the end-to-end routing of the request works successfully.

nothing changes to the default route. This change is being made so that cli is more consistent with what engine supports. Their request was specific to rest-path. so nothing changes for them.

I think what Shyam was trying to say is we need to inform SWA CLI team of this new capability of being able to customize graphql path too in case they would like it to expose to their customers.

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making CLI consistent with engine and fixing the gaps

Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

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

LGTM!

@abhishekkumams abhishekkumams merged commit cfdb24c into main Mar 20, 2023
@abhishekkumams abhishekkumams deleted the dev/abhishekkuma/allow_setting_rest_and_graphql_runtime_flags branch March 20, 2023 08:25
abhishekkumams added a commit that referenced this pull request Mar 30, 2023
## Why make this change?

- Closes #1276, #1310
- To allow users to disable rest and graphQL endpoints globally using
CLI
  - To allow users to provide custom graphQL path through CLI.

## What is this change?

- adding 3 new options to CLI `init` command:
  - `--graphql.path` : To provide custom graphql path
  - `--rest.disabled`: To disable rest endpoints globally 
  - `--graphql.disabled`: To disable graphql endpoints globally

## Why did i use `--rest.disabled/--graphql.disabled` instead of
`--rest.enabled/--graphql.enabled` ?
- the boolean options do not accept values. so, we cannot do
--rest.enabled false. Just using this flag means true.
- By default REST/GraphQL points are enabled, hence it's more convinient
to use `--rest.disabled/--graphql.disabled` only when we want to disable
it. reduces the length of command.

## How was this tested?

- [x] Integration Tests
- [x] Unit Tests

## Sample Request(s)

dab init --database-type mssql
```json
"runtime": {
    "rest": {
      "enabled": true,
      "path": "/api"
    },
    "graphql": {
      "allow-introspection": true,
      "enabled": true,
      "path": "/graphql"
    },
...
  }
```

dab init --database-type mssql --rest.disabled --graphql.disabled
```json
"runtime": {
    "rest": {
      "enabled": false,
      "path": "/api"
    },
    "graphql": {
      "allow-introspection": true,
      "enabled": false,
      "path": "/graphql"
    },
...
  }
```

dab init --database-type mssql --rest.disabled --graphql.path /mygraphql

```json
"runtime": {
    "rest": {
      "enabled": false,
      "path": "/api"
    },
    "graphql": {
      "allow-introspection": true,
      "enabled": true,
      "path": "/mygraphql"
    },
...
  }
```

![image](https://user-images.githubusercontent.com/102276754/223048304-4f303080-ecb3-4ca6-aa0f-2d171e5889e7.png)
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.

[Feature Request]: Allow providing custom GraphQL path through CLI Allow setting rest and graphql runtime flags through CLI

6 participants