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

Receive files outside the classpath in quarkus-liquibase #39720

Merged
merged 2 commits into from
Apr 1, 2024
Merged

Receive files outside the classpath in quarkus-liquibase #39720

merged 2 commits into from
Apr 1, 2024

Conversation

juanjogv
Copy link
Contributor

@juanjogv juanjogv commented Mar 26, 2024

This PR is the continuation of an improvement made in quarkus-liquibase-mongodb extension which was replicated in quarkus-liquibase extension, more information about it can be found in this issue.

This comment has been minimized.

juanjogv added 2 commits April 1, 2024 08:00
…elog location

uploading an adjustment

adding ClassLoaderResourceAccessor to the compositeResourceAccessor

formatting code
Added the classpath to the CompositeResourceAccessor of LiquibaseMongodbFactory, this way even if you specify a custom searchpath you will still be able to reference some file inside the resources folder.
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

quarkus-bot bot commented Apr 1, 2024

Status for workflow Quarkus CI

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

✅ 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.

/**
* The search path for DirectoryResourceAccessor
*/
@ConfigItem(defaultValue = "/")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually wondering if this is a good default... Does this mean it will start searching at the root of the file system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you check here Im validating that if it is set by default that / and if the changelog property does not have the prefix filesystem: it refers to classpath

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I saw that but I have to admit it's confusing. Anyway, let's leave it like this for now, but I assume that using Optional<List<String>> for searchPath would be better.

Copy link
Contributor Author

@juanjogv juanjogv Apr 1, 2024

Choose a reason for hiding this comment

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

I did it this way because at the time I could not find how to declare the default list as an empty list in the build time config, because if I put a default value of "" in the tag I got an error related to null.

Copy link
Contributor

Choose a reason for hiding this comment

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

    /**
     * The search path for DirectoryResourceAccessor
     */
    @ConfigItem
    public Optional<List<String>> searchPath;

should work, no?

Copy link
Contributor Author

@juanjogv juanjogv Apr 1, 2024

Choose a reason for hiding this comment

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

I can try and made the change if u prefer it that way, no biggie for me. As this is my first time contributing in quarkus I am not very clear on how to do certain things and that was a solution that I found viable at the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's merge this and you can try and update both configs with my suggestion. If all goes well, you can open a new PR

@geoand geoand merged commit 274f100 into quarkusio:main Apr 1, 2024
19 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 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.

2 participants