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

Add rasa readiness probe and configurable path #341

Merged
merged 3 commits into from
Jun 20, 2023
Merged

Add rasa readiness probe and configurable path #341

merged 3 commits into from
Jun 20, 2023

Conversation

miraai
Copy link
Contributor

@miraai miraai commented Jun 14, 2023

No description provided.

@miraai miraai requested review from camattin, amn41 and rasa-jmac June 14, 2023 09:44
@miraai miraai requested a review from rasa-jmac June 15, 2023 14:46
@amn41
Copy link

amn41 commented Jun 15, 2023

thanks so much for your work on this, @miraai ! We have a call tomorrow at 10am with the architecture team at Telekom. Do you think we can have a release cut by then?

charts/rasa-x/values.yaml Outdated Show resolved Hide resolved
@miraai
Copy link
Contributor Author

miraai commented Jun 16, 2023

thanks so much for your work on this, @miraai ! We have a call tomorrow at 10am with the architecture team at Telekom. Do you think we can have a release cut by then?

Could you also confirm if we really want default to be /status based on @camattin feedback here? The path is configurable now so you can set it up with the customer to be /status if needed, but it might be counterproductive to have it as a default value.
cc @rasa-jmac

@rasa-jmac
Copy link
Contributor

I think based on the fact /status is authenticated, we should be leaving the default as / because otherwise we'll be shipping them in a default broken state which I don't think is in anyone's interest? @amn41

@amn41
Copy link

amn41 commented Jun 19, 2023

Thanks! That makes sense to me, provided we can make it clear in the docs how to configure this. I'm actually not sure how you would provide the token to be used for the /status endpoint in case that's the one you wanted to use. But good with me so long as the docs are clear.

@miraai miraai merged commit ad4f261 into main Jun 20, 2023
@miraai miraai deleted the infra-403 branch June 20, 2023 10:05
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.

4 participants