-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Issue #11747: Migrate codenarc to travis CI #12349
Issue #11747: Migrate codenarc to travis CI #12349
Conversation
a73c088
to
4775147
Compare
.ci/codenarc.sh
Outdated
@@ -1,6 +1,5 @@ | |||
#!/bin/bash | |||
# Attention, there is no "-x" to avoid problems on Travis | |||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to exit if groovy ./.ci/codenarc.groovy
fails, we still need to echo the reason for failure, for example, if groovy is not installed or JAVA_HOME
is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required to fail fast and do not continue execution I'd something existing with error code.
Please share more details and log on this update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually set -e
can stay, error in groovy installation or JAVA_HOME
would be shown in before_script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, placing -e
in Travis level code is damaging it's execution, but in script it is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items
.ci/codenarc.sh
Outdated
@@ -1,6 +1,5 @@ | |||
#!/bin/bash | |||
# Attention, there is no "-x" to avoid problems on Travis | |||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required to fail fast and do not continue execution I'd something existing with error code.
Please share more details and log on this update
e0955b4
to
4fc8b70
Compare
4fc8b70
to
e9fb950
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to merge
before_script: | ||
- | | ||
if [[ $TRAVIS_CPU_ARCH == 'ppc64le' ]]; then | ||
export JAVA_HOME=/usr/lib/jvm/adoptopenjdk-11-hotspot-ppc64el |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romani Isn't this better suited to actually be in ./.ci/travis.sh install-adoptium-jdk
as it is connected to the JRE and to keep it all together as needed for travis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ok to keep installation details in travis.yml.
I try to keep Travis specific validations in travis.sh (but potentially with extra effort, they can be moved to other CI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep evidence of weird architecture in yml file, to remind us of this when adding new jobs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the one comment.
Resolves #11747
Proof of working:
Failing due to extra import - https://app.travis-ci.com/github/checkstyle/checkstyle/jobs/586922249