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

[ATO-1661] Add --endpoints flag to rasa-sdk cli #1072

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

Tawakalt
Copy link
Contributor

@Tawakalt Tawakalt commented Feb 5, 2024

Proposed changes:

Screenshot 2024-02-05 at 13 57 51

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation in the rasaHQ/rasa
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@Tawakalt Tawakalt force-pushed the ATO-1661-add-endpoints-arg-to-rasa-sdk-cli branch from f80680b to c928e65 Compare February 5, 2024 14:17
parser.add_argument(
"--endpoints",
default=DEFAULT_ENDPOINTS_PATH,
help="Configuration file for tracing as a yml file.",
Copy link
Member

Choose a reason for hiding this comment

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

Minor comment: I expect the assistant endpoints file would be used which contains more details than just tracing config. I'd rephrase this to reflect that, unless you recommend separating the tracing config in a diff config file, in which case we should rename the arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 I left one improvement suggestion for the test.

import rasa_sdk.endpoint as ep


def test_arg_parser_endpoints():
Copy link
Member

Choose a reason for hiding this comment

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

Final comment: can we parametrize this test so that it checks for default in one test, and for the specified param in another case?

@Tawakalt Tawakalt force-pushed the ATO-1661-add-endpoints-arg-to-rasa-sdk-cli branch from 66e7a0d to e9d7183 Compare February 5, 2024 15:16
@Tawakalt Tawakalt merged commit 33b8fa4 into main Feb 5, 2024
16 checks passed
@Tawakalt Tawakalt deleted the ATO-1661-add-endpoints-arg-to-rasa-sdk-cli branch February 5, 2024 15:23
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