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.

Sign up for GitHub

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

Jump to bottom

Auto PR - Kalix Runtime version 1.1.23 #1859

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Auto PR - Kalix Runtime version 1.1.23 #1859

merged 1 commit into from
Nov 20, 2023

Conversation

kalix-bot
Copy link
Collaborator

After the Runtime version reaches prod, please mark as ready, review and merge. 5a3b5c88f026646730846a1c31bb6e8e2d6192da

@kalix-bot kalix-bot requested a review from patriknw November 15, 2023 10:55
@patriknw
Copy link
Contributor

Let's merge #1853 first, because that includes the signature change of the HealthCheck

patriknw
patriknw approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM

...ven-archetype-event-sourced-entity/src/main/resources/archetype-resources/docker-compose.yml
@@ -5,7 +5,7 @@
version: "3"
services:
kalix-proxy:
image: gcr.io/kalix-public/kalix-proxy:1.1.22]]#
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Member

@efgpinto efgpinto Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it came from https://github.com/lightbend/kalix-jvm-sdk/pull/1847/files#diff-ae45dca3d7d520f1111eb3e1dd9929349ac20dd90a0fa033ab684918e037ca94R8-R10

And I guess now the logic we use to open the PR with the bot is not smart enough to let the "]]#" there.. although I wonder what is that doing? @aludwiko are the "]]#" really required here?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's a part of the velocity template, I need to escape this part to use a variable a line below

Copy link
Member

Choose a reason for hiding this comment

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

I assume we cannot do the escaping piece on the line below though?

Something like: ]]#container_name: ${artifactId} ?

Copy link
Contributor

Choose a reason for hiding this comment

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

well, maybe we can, I haven't thought about it

Copy link
Member

Choose a reason for hiding this comment

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

fix here

@patriknw patriknw marked this pull request as ready for review November 15, 2023 14:25
@patriknw patriknw force-pushed the bump-proxy-version-1.1.23 branch from a9c3cf5 to 5acbdef Compare November 15, 2023 14:26
@patriknw patriknw marked this pull request as draft November 15, 2023 14:27
@octonato octonato marked this pull request as ready for review November 20, 2023 13:29
@octonato
Copy link
Member

We can merge this one already. 1.1.23 is in Prod

@octonato octonato merged commit ab69f85 into main Nov 20, 2023
65 checks passed
@octonato octonato deleted the bump-proxy-version-1.1.23 branch November 20, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aludwiko aludwiko aludwiko left review comments

@efgpinto efgpinto efgpinto left review comments

@patriknw patriknw patriknw approved these changes

Assignees
No one assigned
Labels
java-sdk java-sdk-protobuf kalix-runtime Runtime and SDKs sub-team scala-sdk-protobuf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kalix-bot @patriknw @octonato @efgpinto @aludwiko