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

Enable Liquibase includeAll in Native Image #42095

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Conversation

gcw-it
Copy link
Contributor

@gcw-it gcw-it commented Jul 24, 2024

Fixes #16292

This patch creates a FileSystem in the native image to enable access to the Liquibase changelog resources.

gsmet
gsmet previously requested changes Jul 25, 2024
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I added a small nitpick about packages but I will let @zakkak have a look as I'm not very familiar with this GraalVM feature.

import liquibase.resource.PathResource;
import liquibase.resource.Resource;

public class NativeImageResourceAccessor extends AbstractPathResourceAccessor {
Copy link
Member

Choose a reason for hiding this comment

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

This one needs to be in the runtime subpackage as it's not API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@gsmet gsmet requested a review from zakkak July 25, 2024 07:05
@gcw-it gcw-it force-pushed the pr_16292 branch 2 times, most recently from 5d108be to 3349324 Compare July 25, 2024 10:10
@gsmet gsmet dismissed their stale review July 25, 2024 10:57

Addressed!

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM.

Please have a look at the suggestion regarding the method detecting if we are in a native image.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

@gcw-it thank you. Could you please also squash the two commits?

@gcw-it
Copy link
Contributor Author

gcw-it commented Jul 26, 2024

Done.

@zakkak zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 26, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 26, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit bf6232b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@zakkak zakkak merged commit f7dee26 into quarkusio:main Jul 26, 2024
21 checks passed
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jul 26, 2024
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Jul 26, 2024
@gcw-it gcw-it deleted the pr_16292 branch July 26, 2024 14:49
@gsmet gsmet modified the milestones: 3.14 - main, 3.13.1 Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Liquibase includeAll does not work in native mode
3 participants