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-2731] Initialise gRPC health check service #1129

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

radovanZRasa
Copy link
Contributor

Makefile

Added new target in Makefile which will build docker image with dev dependencies.

Initialise gRPC health check service

Health check for gRPC server is initialised.
Added Integration tests for gRPC server:

  • when run in standalone mode - target to run tests is added to Makefile
  • when run as docker container - targets to setup env and run tests are added to Makefile
    Added CI steps which will run integration tests, using targets from Makefile.

Generate SSL certificates

Added Makefile with targets to generate SSL certificates, located under certs directory.
Generated certificates will be placed in certs directory because that will prevent accidental commit of the certificates (see cert/.gitignore).
Move generated certificates and keys you need, to the directory of integration test for which they will be used.

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)

@radovanZRasa radovanZRasa requested a review from a team as a code owner August 6, 2024 21:45
@radovanZRasa radovanZRasa requested review from miraai and removed request for a team August 6, 2024 21:45
@radovanZRasa radovanZRasa force-pushed the bugfix/ATO-2731-grpc-health-check branch 8 times, most recently from a712bea to be82714 Compare August 7, 2024 08:10
@radovanZRasa radovanZRasa force-pushed the bugfix/ATO-2731-grpc-health-check branch from be82714 to 14b925b Compare August 7, 2024 08:39
Dockerfile.dev Show resolved Hide resolved

return grpc.insecure_channel(
target=f"{GRPC_HOST}:{GRPC_NO_TLS_PORT}",
compression=grpc.Compression.Gzip)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the arguments be all in one line? Wondering how this passed the Code Quality 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They would go beyond 88 character line limit if all arguments were in one line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Running ruff format for this code formats it as

    return grpc.insecure_channel(
        target=f"{GRPC_HOST}:{GRPC_NO_TLS_PORT}", compression=grpc.Compression.Gzip
    )

with the line length of 84.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick anyways. I know if this was rasa-private that the Code Quality would fail for this. Just raising it for the concern if the Code Quality check is proper for rasa-sdk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, but we are using black as formatter in Rasa SDK ATM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a PR to switch to ruff and remove black alltogether.

Copy link
Contributor

@aleksandarmijat aleksandarmijat left a comment

Choose a reason for hiding this comment

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

Very clean CI 💯

@radovanZRasa radovanZRasa merged commit 26de1e4 into 3.9.x Aug 12, 2024
17 checks passed
@radovanZRasa radovanZRasa deleted the bugfix/ATO-2731-grpc-health-check branch August 12, 2024 08:23
radovanZRasa added a commit that referenced this pull request Aug 19, 2024
* Initialize the built-in gRPC health service

* Do not consider deleted files for filtering

* Remove comment which indicates that grpc code-in-sync check is not run on CI

* Prefix private functions from gRPC server module with underscore

* Add integration tests for gRPC

* Add changelog for healthcheck service in gRPC mode
radovanZRasa added a commit that referenced this pull request Aug 19, 2024
* Initialize the built-in gRPC health service

* Do not consider deleted files for filtering

* Remove comment which indicates that grpc code-in-sync check is not run on CI

* Prefix private functions from gRPC server module with underscore

* Add integration tests for gRPC

* Add changelog for healthcheck service in gRPC mode
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