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

Fix update sample version when releasing #596

Merged
merged 1 commit into from
May 17, 2024

Conversation

iocanel
Copy link
Collaborator

@iocanel iocanel commented May 16, 2024

Follow up fix for: #556

@iocanel iocanel requested a review from a team as a code owner May 16, 2024 13:56
@geoand
Copy link
Collaborator

geoand commented May 16, 2024

Did you run this to see that is make the change we want (to the quarkus-langchain4j.version property and not the version of the project itself)?

@jmartisk
Copy link
Collaborator

jmartisk commented May 16, 2024

Huhm, how could I be so dumb to not notice..
Yeah, to update the property, you need to do

 ./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:update-property -Dproperty=quarkus-langchain4j.version -DnewVersion=${{steps.metadata.outputs.current-version}} -f samples/email-a-poem

.. for all samples

Changing between -f or -pl for choosing the submodule doesn't seem to make a difference

./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:set -DnewVersion=${{steps.metadata.outputs.current-version}} -f samples/chatbot
./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:set -DnewVersion=${{steps.metadata.outputs.current-version}} -f samples/csv-chatbot
./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:set -DnewVersion=${{steps.metadata.outputs.current-version}} -f samples/cli-translator
./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:set -DnewVersion=${{steps.metadata.outputs.current-version}} -f samples/chatbot-easy-rag
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea to use some bash to automate it?

for module in samples/*; do echo $module; done

Make sure you define the shell as bash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought about that too, but there's one sample (the jbang one) that isn't maven based so we'd have to exclude it, and that felt ugly enough for me to persuade me to go with hardcoding the list of modules. But of course we can do it.

Copy link
Member

Choose a reason for hiding this comment

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

If you insist:

for pom in `find samples/ -name pom.xml`; do module=$(dirname "$pom"); echo $module; done

Copy link
Collaborator

@geoand geoand May 17, 2024

Choose a reason for hiding this comment

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

@iocanel do you want to update the PR as per Guillaume and Jan's suggestions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the PR with @gsmet instructions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have added a comment below

./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:set -DnewVersion=${{steps.metadata.outputs.current-version}} -pl samples/cli-translator
./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:set -DnewVersion=${{steps.metadata.outputs.current-version}} -pl samples/chatbot-easy-rag
for pom in `find samples/ -name pom.xml`;
./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:set -DnewVersion=${{steps.metadata.outputs.current-version}} -f $(dirname "$pom");
Copy link
Collaborator

@geoand geoand May 17, 2024

Choose a reason for hiding this comment

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

Shouldn't it be

./mvnw org.codehaus.mojo:versions-maven-plugin:2.16.2:update-property -Dproperty=quarkus-langchain4j.version ...

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use set-property.

@geoand geoand merged commit e7a911a into quarkiverse:main May 17, 2024
12 checks passed
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