Skip to content

feat: samples uses project.version as sdk version#156

Merged
octonato merged 4 commits intomainfrom
rgc/update-pom-versions-script
Jul 16, 2021
Merged

feat: samples uses project.version as sdk version#156
octonato merged 4 commits intomainfrom
rgc/update-pom-versions-script

Conversation

@octonato
Copy link
Copy Markdown
Member

Those changes will allows us to:

  • change the sdk version in all mvn projects in one shot (changes can be pushed or not)
  • samples use its own project.version as sdk version
  • remove passing -D in CI, it suffice to use mvn versions:set

@octonato octonato force-pushed the rgc/update-pom-versions-script branch from 2a8efbc to 0688344 Compare July 16, 2021 13:08
Copy link
Copy Markdown
Member Author

@octonato octonato left a comment

Choose a reason for hiding this comment

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

left a few comments to explain the changes

Comment thread .circleci/config.yml
cd maven-java
mvn -B archetype:generate -DgroupId=com.example -DartifactId=counter-value-entity -DarchetypeGroupId=com.akkaserverless -DarchetypeArtifactId=akkaserverless-maven-archetype -DarchetypeVersion=$SDK_VERSION
cd counter-value-entity
mvn -B -Dakkaserverless-sdk.version=$SDK_VERSION compile
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we don't need this anymore since #154, the archetype comes with the right version for the sdk

Comment thread .circleci/config.yml Outdated
cd samples/java-valueentity-shopping-cart
echo "Running mvn with SDK version: '$SDK_VERSION'"
mvn -Dakkaserverless-sdk.version=$SDK_VERSION -Dakkaserverless-maven-plugin.version=$SDK_VERSION verify -Pit
mvn versions:set -DnewVersion=$SDK_VERSION verify -Pit
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

because the samples now use ${project.version} as the sdk version, it suffice to call mvn versions:set.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, so this works because we have <akkaserverless-sdk.version>${project.version}</akkaserverless-sdk.version> - which is perhaps OK for us, but would definitely be different for users.

I'm OK with it, but TBQH I don't see the advantage of using versions:set over -D for the samples. We're not publishing those, are we?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, we are not publishing. But we do link them from the samples in the console. And users may checkout such a sample and run locally. In that case, we want the version to more or less aligned.

Also, if we use mvn versions:set we can from the script update the samples to run them locally for our own tests. Otherwise, we need to pass -D to maven, but then, if you want to open such a sample in intellij, you want it to have a fixed sdk version, not something to change with -D.

but would definitely be different for users.

That won't be different for users since this is just a sample. When a user starts a new project from the archetype, they get the sdk version fixed and independent from their project version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we want the version to more or less aligned.

We want the SDK version in the sample to be the latest, agreed!

if you want to open such a sample in intellij, you want it to have a fixed sdk version, not something to change with -D

I can see how that's useful. Using -D means the pom.xml don't have to change all the time (marking the git repo dirty), but I guess there's no easy way to have both advantages ;)

That won't be different for users since this is just a sample.

I meant, it's a bit confusing that the sample is different from what the user actually gets when they start a new project from the archetype. Not too big of a deal though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it's a bit confusing that the sample is different from what the user actually gets when they start a new project from the archetype

Yeah, it's true.
We can try to improve it. We can probably do some sed on the pom and fix the version from that same script.

Comment on lines -20 to -21
<akkaserverless-sdk.version>0.7.0-beta.10</akkaserverless-sdk.version>
<akkaserverless-maven-plugin.version>0.7.0-beta.10</akkaserverless-maven-plugin.version>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sdk.version is enough. All artifacts must align to it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had that on my mental note list but forgot to talk to anyone about it :)

Comment thread updatePomVersions.sh
Comment on lines +1 to +4
# this script updates all maven projects versions (maven-java and samples) to align with the current sdk version
# if SDK_VERSION env var is defined, it will use it, otherwise it will take the version from sbt
# after running this script, you may run local tests or simply send a PR with the updates.
# the script can also run publishM2 and mvn install to generate the artifacts with the new version.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

With this script we can test locally, but also update the pom after a release. We don't need to do it on each release though.

Copy link
Copy Markdown
Contributor

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Neat

@octonato
Copy link
Copy Markdown
Member Author

@raboof, I will merge this one as such and improve it later one using sed. I would like to have this in for now because it's useful for the current dev cycle we are in.

@octonato octonato merged commit cbf545d into main Jul 16, 2021
@octonato octonato deleted the rgc/update-pom-versions-script branch July 16, 2021 14:34
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.

3 participants