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: add help endpoint. #311

Merged

Conversation

princerajpoot20
Copy link
Member

@princerajpoot20 princerajpoot20 commented Jun 14, 2023

Resloves #144

Description
Adding an endpoint, help/{command}, to return instructions for a given command to the user. For example, help/generate should return available parameters, such as available templates, etc.

To Do

  • Implement help.controller.ts
  • Update openapi.yaml
  • Add unit test for this using Jest and Supertest

closes #144

Copy link
Member

@BOLT04 BOLT04 left a comment

Choose a reason for hiding this comment

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

Looking good @princerajpoot20 , it's a great start 👍
We need some unit tests later too, but for now I've left some feedback to consider.

openapi.yaml Outdated Show resolved Hide resolved
src/controllers/help.controller.ts Outdated Show resolved Hide resolved
src/controllers/help.controller.ts Outdated Show resolved Hide resolved
src/controllers/help.controller.ts Outdated Show resolved Hide resolved
src/controllers/help.controller.ts Outdated Show resolved Hide resolved
@BOLT04
Copy link
Member

BOLT04 commented Jun 16, 2023

Also the CI checks are failing, can you look into those later @princerajpoot20 ?

@princerajpoot20
Copy link
Member Author

Also the CI checks are failing, can you look into those later @princerajpoot20 ?

Sure, I will take a look at the CI checks @BOLT04

@princerajpoot20 princerajpoot20 requested a review from BOLT04 June 18, 2023 18:06
@princerajpoot20
Copy link
Member Author

princerajpoot20 commented Jun 20, 2023

Changes

  • I have changed the parameter passing from the request body to the URL.
  • The commands file is pointing to the main CLI's usage.md file.

When only help endpoint is called.

Screenshot 2023-06-20 at 7 49 50 PM

get(help/asyncapi/generate)

Screenshot 2023-06-20 at 8 21 58 PM

get(/help/asyncapi/generate/modles/fromTemplate/ASYNCAPI/template)

Screenshot 2023-06-20 at 8 24 19 PM

Updated openapi.yml file for url path

@BOLT04 Could you please review this.

@BOLT04
Copy link
Member

BOLT04 commented Jul 8, 2023

Changes

* I have changed the parameter passing from the request body to the URL.

* The commands file is pointing to the main CLI's usage.md file.

When only help endpoint is called.

Screenshot 2023-06-20 at 7 49 50 PM ### `get(help/asyncapi/generate)` Screenshot 2023-06-20 at 8 21 58 PM ### `get(/help/asyncapi/generate/modles/fromTemplate/ASYNCAPI/template)` Screenshot 2023-06-20 at 8 24 19 PM ### get(/help/asyncapi/diff/old/new) Screenshot 2023-06-20 at 8 25 56 PM # Parameter can also be passed with `" " (space)` in url. #### `get(/help/asyncapi generate modles fromTemplate ASYNCAPI template)` Screenshot 2023-06-20 at 8 24 50 PM ## The following implementation has two ways of passing parameters in the URL:
* Passing parameters in the URL, separated by `"/"`.

* Passing parameters in the URL, separated by spaces. The whole command, separated by spaces, should be passed after `/help/`.

@BOLT04 Could you please review this.

I'll separate my feedback in sections 🙂.

API response for /help

The response for the help endpoint without any parameters can be inspired by the CLI usage documentation. But in our case this is a HTTP API, so the response should have URI links for all available help endpoints. Please change the response that you are using to be appropriate with the API context.

Instead of asyncapi generate, we should have this URI in the response /help/generate. You can make them relative links or absolute links. But if you make them absolute links please don't hardcode "localhost" in the URI 😅. The API in production has the domain api.asyncapi.com, so the absolute URI would be https://api.asyncapi.com/help/generate, and the relative URI would be /help/generate 👍

The API response should be JSON

@princerajpoot20 the screenshots you're sending make it seem the API's response is markdown or something else. They should be JSON like you have on the openapi.yml 😃. For example this information can be deleted:
imagem

Also bear in mind @princerajpoot20, we just want this endpoint to be similar to the CLI commands usage, not exactly the same. The information this API endpoint returns needs to be in the context of the server-api, not the CLI. For example the way we use the generate endpoint in the server API is by sending a JSON payload, like this:

{
    "asyncapi": {
        "asyncapi": "2.2.0",
        "info": {
            "title": "Test Service",
            "version": "1.0.0"
        },
        "channels": {}
    },
    "template": "@asyncapi/html-template"
}

So when the help endpoint receives the parameter generate in the URL, the JSON response needs to specify some details like you are already providing (just not in markdown), and then the possible values users can send for each field in the JSON payload of the generate endpoint. For example, for template you would send the enum list that is present on the openapi.yml file of the server-api.
Please take this into consideration for your implementation @princerajpoot20.

URL Parameters can not have spaces

The URLs can't have spaces, but you also shouldn't need to have spaces. Sending generate modles fromTemplate ASYNCAPI template isn't necessary, like I said the help endpoint isn't for the CLI, it's for server-api. So you can just have generate and provide the details about what templates are supported in the JSON response, and what format should be the asyncapi field.

Also, the flags information you are returning isn't applicable for server-api

In general, change the API responses to be in context of server-api, not the CLI commands 🙂

@asyncapi-bot
Copy link
Contributor

Hello, @BOLT04! 👋🏼

    I'm 🧞🧞🧞 Genie 🧞🧞🧞 from the magic lamp. Looks like somebody needs a hand!

    At the moment the following comments are supported in pull requests:

    - `/ready-to-merge` or `/rtm` - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
    - `/do-not-merge` or `/dnm` - This comment will block automerging even if all conditions are met and ready-to-merge label is added
    - `/autoupdate` or `/au` - This comment will add `autoupdate` label to the PR and keeps your PR up-to-date to the target branch's future changes. Unless there is a merge conflict or it is a draft PR.

@princerajpoot20
Copy link
Member Author

The API response should be JSON

@princerajpoot20 the screenshots you're sending make it seem the API's response is markdown or something else. They should be JSON like you have on the openapi.yml 😃. For example this information can be deleted: ![imagem]

Also bear in mind @princerajpoot20, we just want this endpoint to be similar to the CLI commands usage, not exactly the same. The information this API endpoint returns needs to be in the context of the server-api, not the CLI. For example the way we use the generate endpoint in the server API is by sending a JSON payload, like this:

{
    "asyncapi": {
        "asyncapi": "2.2.0",
        "info": {
            "title": "Test Service",
            "version": "1.0.0"
        },
        "channels": {}
    },
    "template": "@asyncapi/html-template"
}

So when the help endpoint receives the parameter generate in the URL, the JSON response needs to specify some details like you are already providing (just not in markdown), and then the possible values users can send for each field in the JSON payload of the generate endpoint. For example, for template you would send the enum list that is present on the openapi.yml file of the server-api. Please take this into consideration for your implementation @princerajpoot20.

@BOLT04 Could you explain this more in detail.

@BOLT04
Copy link
Member

BOLT04 commented Jul 14, 2023

The API response should be JSON

@princerajpoot20 the screenshots you're sending make it seem the API's response is markdown or something else. They should be JSON like you have on the openapi.yml 😃. For example this information can be deleted: ![imagem]
Also bear in mind @princerajpoot20, we just want this endpoint to be similar to the CLI commands usage, not exactly the same. The information this API endpoint returns needs to be in the context of the server-api, not the CLI. For example the way we use the generate endpoint in the server API is by sending a JSON payload, like this:

{
    "asyncapi": {
        "asyncapi": "2.2.0",
        "info": {
            "title": "Test Service",
            "version": "1.0.0"
        },
        "channels": {}
    },
    "template": "@asyncapi/html-template"
}

So when the help endpoint receives the parameter generate in the URL, the JSON response needs to specify some details like you are already providing (just not in markdown), and then the possible values users can send for each field in the JSON payload of the generate endpoint. For example, for template you would send the enum list that is present on the openapi.yml file of the server-api. Please take this into consideration for your implementation @princerajpoot20.

@BOLT04 Could you explain this more in detail.

@princerajpoot20 sure, my feedback is on the API's response. We want to provide details about how you'd call these endpoints. For example the generate endpoint, we send the template in the request body to specify the code template we want to generate. The help's API endpoint for generate could be something like this:

{
  "template": [
    "@asyncapi/html-template",
    "@asyncapi/nodejs-template",
    ...
  ]
}

If you're having doubts about how to implement this, ping me on slack 👍

@princerajpoot20
Copy link
Member Author

Hello @BOLT04,

I've implemented the changes we discussed last week, including updating the URL for the AsyncAPI documents. You can check the updated response at this link. I have also added unit tests.

Could you please take a look and provide your feedback?

@asyncapi-bot
Copy link
Contributor

Hello, @princerajpoot20! 👋🏼

    I'm 🧞🧞🧞 Genie 🧞🧞🧞 from the magic lamp. Looks like somebody needs a hand!

    At the moment the following comments are supported in pull requests:

    - `/ready-to-merge` or `/rtm` - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
    - `/do-not-merge` or `/dnm` - This comment will block automerging even if all conditions are met and ready-to-merge label is added
    - `/autoupdate` or `/au` - This comment will add `autoupdate` label to the PR and keeps your PR up-to-date to the target branch's future changes. Unless there is a merge conflict or it is a draft PR.

openapi.yaml Show resolved Hide resolved
src/controllers/tests/help.controller.test.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
@princerajpoot20
Copy link
Member Author

princerajpoot20 commented Aug 18, 2023

@BOLT04
I have made the changes you have mentioned:

  • add 404 response
  • refactor: Apply Problem schema
  • update to the latest version of axios
  • removed error 422
  • update schema : HelpListResponse, HelpCommandResponse

Updating the Axios version breaks Jest tests. I found this issue here in axios repo. I discovered that adding the configuration to jest.config.js works fine.

Here are responses of command.

Could you please take a look and provide your feedback whenever you get a moment 🙂

@princerajpoot20 princerajpoot20 requested a review from BOLT04 August 18, 2023 17:39
@princerajpoot20 princerajpoot20 changed the title feat: improved draft for help endpoint. feat: add help endpoint. Aug 18, 2023
…pCommandResponse, add 404 response, removed error 422
@princerajpoot20 princerajpoot20 force-pushed the improved-draft-help-endpoint branch from f137fdb to ed228cd Compare August 19, 2023 15:56
Copy link
Member

@BOLT04 BOLT04 left a comment

Choose a reason for hiding this comment

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

everything looks fine, just one comment regarding the function getting commands 🙂

src/controllers/help.controller.ts Outdated Show resolved Hide resolved
@princerajpoot20
Copy link
Member Author

@BOLT04 I have made the changes. We are now using getAppOpenAPI to obtain the OpenAPI spec.

Here are the updated responses from the help endpoint.

BOLT04
BOLT04 previously approved these changes Sep 7, 2023
Copy link
Member

@BOLT04 BOLT04 left a comment

Choose a reason for hiding this comment

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

Looks great, awesome job @princerajpoot20 🙂
Curry Dance

@BOLT04
Copy link
Member

BOLT04 commented Sep 7, 2023

@magicmatatjahu pinging you in case you want to give some feedback on this PR 👍

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@BOLT04
Copy link
Member

BOLT04 commented Sep 15, 2023

@princerajpoot20 great job 👍
/rtm

@asyncapi-bot asyncapi-bot merged commit 81cd935 into asyncapi:master Sep 15, 2023
7 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@princerajpoot20
Copy link
Member Author

@BOLT04 Thank you for all the help and guidance you provided 🙇🏻‍♂️. I truly appreciate how you took the time to discuss ideas and give me useful feedback. Because of you, I've learned and grown a lot in the process. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add help/{command} endpoint
3 participants