-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update Google Cloud SQL guide #43901
Conversation
This comment has been minimized.
This comment has been minimized.
🙈 The PR is closed and the preview is expired. |
|
||
[source,properties] | ||
---- | ||
quarkus.native.additional-build-args=--initialize-at-run-time=jnr.ffi.provider.jffi.NativeFinalizer$SingletonHolder\,com.kenai.jffi.internal.Cleaner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small typo here, there should be double backslashes to escape the comma. See: #6710 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loicmathieu @daniel-pettersson-pricer do you think it would make sense to have an extension here: https://github.com/quarkiverse/quarkus-google-cloud-services to handle this properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @daniel-pettersson-pricer. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet it's not the first time we asked ourself about an extension for CloudSQL but the issue is that as it would not be tested on a real instance for practical reason (cost), this would probably not catch this kind of errors so I'm not sure it's worth the work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that would be a good way to capitalize on things. I agree it's not going to magically fix the reported issues but at least people could get the latest fixes just by updating.
Now it's just an idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked here: quarkiverse/quarkus-google-cloud-services#696
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks nice. Just the issue with the escaping of the comma in the additional-build-args that I wrote about in a comment that need to be addressed
5f65fc0
to
489f919
Compare
Status for workflow
|
@daniel-pettersson-pricer thanks for the thorough review, let's get this merged and backported. |
Closes #43876
@daniel-pettersson-pricer can you please review?