Validate all example and sample applications#1059
Conversation
marcospereira
force-pushed
the
mp/check-examples-using-java-8-and-11
branch
4 times, most recently
from
43276ca to
98c7c79
Compare
marcospereira
force-pushed
the
mp/check-examples-using-java-8-and-11
branch
from
98c7c79 to
00b59a6
Compare
marcospereira
force-pushed
the
mp/check-examples-using-java-8-and-11
branch
6 times, most recently
from
58a1005 to
6f8de0c
Compare
marcospereira
force-pushed
the
mp/check-examples-using-java-8-and-11
branch
from
6f8de0c to
94464de
Compare
marcospereira
force-pushed
the
mp/check-examples-using-java-8-and-11
branch
from
94464de to
96b20f2
Compare
| ./build-all.sh +test | ||
| run: ./scripts/build-core.sh +test | ||
|
|
||
| build-and-test-sbt-examples: |
There was a problem hiding this comment.
Split into different jobs so that they can run independently.
| .getOrElse("0.0.0-SNAPSHOT")}"""") | ||
| } | ||
|
|
||
| lazy val writeVersionToFile = taskKey[Unit]("Write project version to file") |
There was a problem hiding this comment.
This helps to have a consistent version that will be used when running the tests for all the samples. The file will later be read by the build shell scripts.
The goal here is to avoid tasks that change files to break dynver (used as a fallback by the samples) since it generates another version if the git repository is dirty.
There was a problem hiding this comment.
Please remove this customization.
The goal here is to avoid tasks that change files to break dynver
That's the reason why we should keep it as it was, the tasks should not change files to while running, otherwise, we want the CI to fail.
There was a problem hiding this comment.
It will fail anyway, but instead of failing with unresolved artifacts for the next sample, it would fail for the right reason if that is the case.
This is also relevant when running the Maven examples where we need the CLOUDFLOW_VERSION env since we don't have dynver there. Or is there an option for Maven samples?
There was a problem hiding this comment.
for Maven something like:
export CLOUDFLOW_VERSION=$(sbt -no-colors --error 'print version')should make it work.
It will fail anyway
If the git repo is not dirty the version is always the same.
But maybe I'm misunderstanding, can you elaborate? or maybe is faster to go through in a sync meeting?
marcospereira
left a comment
marcospereira
left a comment
There was a problem hiding this comment.
@RayRoestenburg, this grew a lot beyond my initial intention. Most of the changes are due to code format, though. Some other changes are small compilation issues in the samples code, which is the original idea behind this PR.
I'm leaving some comments on points you may want to be interested.
|
Sure, let's look at these as possible improvements and pick it up in a support cycle. @andreaTP might also be interested in these changes |
andreaTP
left a comment
andreaTP
left a comment
There was a problem hiding this comment.
I will review this better tomorrow, overall I really like the separation of the bash scripts, thanks @marcospereira !
The current issue with this PR is how it plays with dynver we should be able to avoid fixing the version.
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v2.3.3 | ||
| - uses: actions/checkout@v2 |
There was a problem hiding this comment.
Have you tested this workflow in isolation to verify this change?
I'm pretty sure we experienced a few regressions on the versioning here ...
There was a problem hiding this comment.
I have not. How can I do that?
There was a problem hiding this comment.
you can hack the workflow in a branch to be triggered on push and verify that the output (e.g. the version) is correct and cosistent
| .enablePlugins(CloudflowApplicationPlugin) | ||
| .settings( | ||
| cloudflowAkkaBaseImage := "myRepositoryUrl/myRepositoryPath:2.0.10-cloudflow-akka-2.6.6-scala-2.12", | ||
| cloudflowDockerBaseImage := "myRepositoryUrl/myRepositoryPath:2.0.10-cloudflow-akka-2.6.6-scala-2.12", |
There was a problem hiding this comment.
I'm not sure what is going on here ...
There was a problem hiding this comment.
Avoid using the deprecated setting in the example.
marcospereira
force-pushed
the
mp/check-examples-using-java-8-and-11
branch
from
6e6e97d to
25b5841
Compare
andreaTP
left a comment
andreaTP
left a comment
There was a problem hiding this comment.
Added one commit to test examples only on Java 8.
Thanks for the hard work @marcospereira ! LGTM!
3 participants
What changes were proposed in this pull request?
Some of them were not compiled or tested. The script now discovers all apps and tests them.
Why are the changes needed?
To make sure all apps are in good shape, compiling, and building as expected.
Does this PR introduce any user-facing change?
No API changes at all.
How was this patch tested?
Running
./scripts/build-all.sh testso far.