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

Unify Jenkins pipelines, inline scripts and add full-build launch-config #2620

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Apr 20, 2023

This PR is a suggestion to merge the deployment Jenkins pipelines of Xtext as already mentioned in #2224 and to inline the scripts called in them.

The initial reaction was a bit reluctant, nevertheless I think the suggested changes would have the following advantages:

  • Inlining the scripts make the pipelines simpler to read since there is less indirection
  • Merging the deployment pipelines leads to less (mostly duplicated) pipeline code to maintain and the few groovy-'tricks' that are necessary are IMHO relatively simple to understand (there are more complicated things at least in the main pipeline.
  • The Jenkins probably two jobs for different deployments can be merged into one, that runs a nightly deployment each night, controlled by the cron-trigger and if started manually one can select to run a M, RC or GA release.
  • To test modifications you can replay the pipeline in the Jenkins UI with your modifications applied without the need to push any branch.
  • The scripts that are currently called in the workflows are relatively complicated and hard to understand
  • At least the sign and deploy scripts probably cannot run locally since the required environment (variables, credentials, etc.) is probably not available on a local computer
  • The (bash) scripts can only run on Unix (maybe also on Mac) and on Windows only if one installs something Cygwin

To ease local execution of Xtext's maven build I added a M2E Maven launch config for the full-build.
If you want to have configs for other scenarios (maven only, p2 only) I can of course add them.

Of course the (probably) two Jenkins jobs that use the merged Pipelines have to be merged too.

The two commits should not be squashed. Otherwise git will not record the renaming jenkins/milestone-deploy/Jenkinsfile → jenkins/deploy/Jenkinsfile .

@cdietrich cdietrich added this to the Release_2.31 milestone Apr 20, 2023
Jenkinsfile Outdated
-PuseJenkinsSnapshots \
-Dtarget-platform-classifier=xtext-${selectedTargetPlatform()} \
-Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn \
-DaltDeploymentRepository=local::file:./build/maven-repository
Copy link
Member Author

Choose a reason for hiding this comment

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

In the script the value was -DaltDeploymentRepository=local::default::file:./build/maven-repository, but Maven emitted a deprecation warning, therefore I changed that to the recommended value.

Jenkinsfile Outdated
${javaVersion() == 17 ? '' : '--toolchains releng/toolchains.xml -Pstrict-release-jdk'}
mvn -f org.eclipse.xtext.full.releng --batch-mode clean deploy \
${javaVersion() == 17 ? '' : '--toolchains releng/toolchains.xml -Pstrict-release-jdk'} \
-PuseJenkinsSnapshots \
Copy link
Member Author

Choose a reason for hiding this comment

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

The full-build script also defined -Dit-archetype-tests-skip=true, but that is actually not necessary because that variable is initialized with true.

The same applies for the full-deploy script inlined below.

Comment on lines 17 to 25
triggers { // https://jenkins.io/doc/book/pipeline/syntax/#triggers
cron('50 21 * * *') // nightly at 21:50
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested this in my local Jenkins instance and it worked as desired. The job run automatically at the specified time with DEPLOYMENT_TYPE=nightly, and if one trigger an execution manually one has to choose one of the available parameter values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing nightly builds at a fixed schedule, should we do continuous deployment whenever something's merged into main, @cdietrich ?

Not necessarily in the scope of this ticket, though @HannesWell

@LorenzoBettini
Copy link
Contributor

@HannesWell sorry for the delay.

I think this PR can be used to clean up a few things you have already detected.

BUT:

  • I have a strong opinion against merging the Jenkinsfiles. I definitely prefer to maintain several small Jenkinsfiles, each one with a single responsibility (from the Single Responsibility Principle) than having a big one with ifs, whens, or groovy-tricks. This is a context where testing is hard and takes time, and my solution keeps things simple.
  • the prepare-for-milestone.sh again has a single responsibility and will do bad things if you pass it nightly

The two Jenkinsfiles and the script prepare-for-milestone.sh (which is a shell script, but just calls maven commands) have been carefully baked and tested. I'm not willing to update them. For final releases, I have another script (not already committed), separate from the one for milestones and another Jenkinsfile just for final releases. The original Jenkinsfile before repo merge. was a huge 500 lines of unreadable and untestable Jenkinsfile with ifs, switches, embedded shell scripts... I'm not willing to go back to that road. :)

I'm also against inlining the script files as I had already said.
Maybe they can be cleaned up.
I agree they cannot be run from Windows, but I think it's better to document/suggest the Maven commands in an MD file, than to even create a launch file. (For example, I don't think this -f org.eclipse.xtext.full.releng --fae clean verify is a good command to run locally).

Of course, we have to head from @cdietrich and @szarnekow , but just to stress that again (with smile, of course), I'll be strongly against merging the Jenkinsfiles.

@cdietrich
Copy link
Member

i prefer separate Jenkinsfiles.
having scripts files (at least for me) makes it more eady to run locally without c&p big lines from jenkinsfiles

@HannesWell
Copy link
Member Author

HannesWell commented Apr 26, 2023

BUT:

* I have a strong opinion against merging the Jenkinsfiles. I definitely prefer to maintain several small Jenkinsfiles, each one with a single responsibility (from the Single Responsibility Principle) than having a big one with ifs, whens, or groovy-tricks. This is a context where testing is hard and takes time, and my solution keeps things simple.

Personally I would still say that even the merged pipeline has a single responsibility: It deploys the xtext artifacts. I would consider the type of deployment a parameter, as it already is for the milestone-deploy workflow.
Making a distinction between nightly and M, RC and GA deployment seems kind of artifical to me, but if you want to do that, why not create a own pipeline for each M1, M2, RC1, RC2 and GA (exaggeratedly asked :) )?
The merged pipeline has 7 lines more, which I would not consider much bigger than before and at the same time one entire file can be removed (with 54).
Furthermore there are only two very simple groovy 'tricks' in it (that every java developer should understand), everything else is standard Jenkins declarative pipeline syntax. You even just added the same 'trick' to the same pipeline in #2629.
I honestly don't understand why it is bad in the deployment pipeline but ok in the 'main' Jenkinsfile which does far more complex things?

I it is right that the merged pipeline has to be tested, but I don't expect it to be too complicated since the commands within the pipeline are not changed but only the triggers are changed.
Therefore it should be sufficient to create the new job that uses the merged pipeline (or reconfigure one of xtext-monorepo-full-deploy-nightly xtext-monorepo-full-deploy-milestone,
verify that the corresponding parameters are available when selecting 'Build with parameters', try a nightly-build and a milestone build and verify that the right profiles are actived and the env-variables have the right value.
Last but not least you can test that the cron trigger works (and temporarily set it to the time in 5min, get a coffee and check the result).

I can offer to test and demonstrate you that the changed parts work in the M2E Jenkins instance. The entire pipeline does not have to be tested since it did not change.

* the `prepare-for-milestone.sh` again has a single responsibility and will do bad things if you pass it `nightly`

Yes, that's why this stage is guarded with a corresponding when condition.

The two Jenkinsfiles and the script prepare-for-milestone.sh (which is a shell script, but just calls maven commands) have been carefully baked and tested. I'm not willing to update them. For final releases, I have another script (not already committed), separate from the one for milestones and another Jenkinsfile just for final releases. The original Jenkinsfile before repo merge. was a huge 500 lines of unreadable and untestable Jenkinsfile with ifs, switches, embedded shell scripts... I'm not willing to go back to that road. :)

I understand that the pipeline before the repo merge was too complex and that you don't want to go back there. But personally I would say that the suggested change is far from that.
I agree that the in two points the pipeline became locally slightly less simple, but even that should be understandable for a Java developer.
Having two different pipelines can make them simpler locally but tends to make the overall system more complex and harder to understand (for others or maybe even you in a more distant future) because there is much duplication and it takes more time to understand the difference and non-differences between a nightly deployment and all the other deployments.
Of course this always has to be balanced, but in my humble opinion the suggested changes are overall for the better.

Having two different pipelines increases the maintains costs on the long run because every change you do to one pipeline you probably also have to apply to the other one. And if you really want to test all changes before (which is good) you have to test two pipelines instead of one.
And as a bonus you also have one Jenkins job less with this change, which also makes the build infra-structure less complex.

I'm also against inlining the script files as I had already said. Maybe they can be cleaned up. I agree they cannot be run from Windows, but I think it's better to document/suggest the Maven commands in an MD file, than to even create a launch file. (For example, I don't think this -f org.eclipse.xtext.full.releng --fae clean verify is a good command to run locally).

The downside of just documenting the commands is that it can easily become outdated. The good thing about a launch-config is that it is executable. If regularly used one will notice discrepancies quite quickly and one will remember to change it more likely.

I think this PR can be used to clean up a few things you have already detected.

I hope I made my points clear, if you nevertheless prefer to keep most of it as it is, I'm fine with stop discussing if you want to save your time.
Can you then please just list the changes you are fine with to change?

@LorenzoBettini
Copy link
Contributor

@HannesWell, sorry for the delay.

Releasing nightly, a milestone, or an official release are different tasks; for this reason, I really prefer to have separate Jenkinsfiles, which are much easier to test. Having conditionals makes everything harder to maintain and test.

As I said, also the launch configuration does not look good to me, and for the moment I'd do without it.

I'd keep

  • the updated URL for deploying locally
  • the removal of redundant archetype it skip
  • the removal of script files that we are sure we don't use anymore

@HannesWell
Copy link
Member Author

HannesWell commented May 4, 2023

As I said, also the launch configuration does not look good to me, and for the moment I'd do without it.

Can you please elaborate what exactly does not look good in the launch config? At the moment it reflects the full-build script.

I'd keep

* the updated URL for deploying locally

* the removal of redundant archetype it skip

* the removal of script files that we are sure we don't use anymore

OK, I'll update this PR accordingly (but probably not before next week).

Which scripts can then be removed exactly?
Since at least the full-deploy.sh will be left, would you be fine with moving it into the scripts folder, in order to make the root folder a bit less crowed?

Copy link
Contributor

@szarnekow szarnekow left a comment

Choose a reason for hiding this comment

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

Submitting a bunch of older comments

steps {
sh './scripts/prepare-for-milestone.sh ${RELEASE_TYPE}'
sh './scripts/prepare-for-milestone.sh ${DEPLOYMENT_TYPE}'
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I find the terminology milestone here a bit confusing since it's for every release being it milestone, rc, or ga. Not actionable though.

Copy link
Member

Choose a reason for hiding this comment

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

@szarnekow see also #2657

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I find the terminology milestone here a bit confusing since it's for every release being it milestone, rc, or ga. Not actionable though.

In fact, I want to keep releasing of milestones and actual releases separate.

Comment on lines 17 to 25
triggers { // https://jenkins.io/doc/book/pipeline/syntax/#triggers
cron('50 21 * * *') // nightly at 21:50
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing nightly builds at a fixed schedule, should we do continuous deployment whenever something's merged into main, @cdietrich ?

Not necessarily in the scope of this ticket, though @HannesWell

@@ -14,7 +14,12 @@ pipeline {
stages {
stage('Maven Tycho Build and Sign') {
steps {
sh './tycho-sign.sh'
sh '''
Copy link
Contributor

Choose a reason for hiding this comment

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

@LorenzoBettini what is the idea behind the other-experiements folder? Is it supposed to stay or become obsolete prospectively?

Copy link
Contributor

Choose a reason for hiding this comment

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

They are to be considered obsolete! I forgot to remove them

@szarnekow
Copy link
Contributor

@HannesWell We (as in: committers on this project) came to the conclusion that we prefer the separate Jenkinsfiles for the builds rather than the single merged Jenkinsfile. We know that others might have chosen differently, but we are more confident with the current split. The same holds true for the existing shell scripts. Rather than inlining them into the Jenkinsfile, we prefer them to be available as shell scripts that can be called from the command line. Here, we accept the limitations that you mentioned: These might not work on Windows / for all users, and some environment variables might be missing in the average case. But generally, this is the setup we'd like to move forward with.

For sure, we are grateful for your input, and proposed changes might be considered in some other future that we don't know yet, but as of now, we'd like to keep the individual Jenkinsfiles and their accompanying scripts.

@HannesWell
Copy link
Member Author

@HannesWell We (as in: committers on this project) came to the conclusion that we prefer the separate Jenkinsfiles for the builds rather than the single merged Jenkinsfile. We know that others might have chosen differently, but we are more confident with the current split. The same holds true for the existing shell scripts. Rather than inlining them into the Jenkinsfile, we prefer them to be available as shell scripts that can be called from the command line.

Yes, that's fine. It would not have been my choice (surprise), but I accept your decision.
One question before I adapt this PR: Would you be fine with moving the scripts into the scripts folder to have the root less crowded?

Rather than doing nightly builds at a fixed schedule, should we do continuous deployment whenever something's merged into main, @cdietrich ?

Not necessarily in the scope of this ticket, though @HannesWell

For me this is a good proposal. We do that in m2e and it works quite well and is actually not too complicated:
https://github.com/eclipse-m2e/m2e-core/blob/7b340f774365df15715a92f87ca81bad2bc8a0e0/Jenkinsfile#L68-L97

But at least in m2e we have the deployment script in the Jenkinsfile in contrast to Xtext where the deployment is specified pom.xml of the repos (if I have not misunderstood it). So depending on how that should be done it might require some rework.

Performing the deployment on each push would probably make the question about merging the nightly- and release-deploy pipeline obsolete, since it probably has to be done as part of the 'main' pipeline then.

@cdietrich
Copy link
Member

Please note the deployment of milestone and release to maven central needs manual closing of staged repo

@HannesWell
Copy link
Member Author

HannesWell commented May 11, 2023

I now reduced the PR to the changes you requested.

Please note the deployment of milestone and release to maven central needs manual closing of staged repo

If you use the nexus-maven-plugins to deploy to OSSRH you can set the autoReleaseAfterClose property to true to let the plugin close the repo for you.
But I assume that does not apply for nightly builds?

@cdietrich
Copy link
Member

yes. nightly builds are deployed automatically anyway

@cdietrich
Copy link
Member

@HannesWell
Copy link
Member Author

https://ci.eclipse.org/xtext/job/xtext/job/PR-2620/8/console

According to the logs for the failing project the maven-deploy-plugin version 2.7 is used. The reason is that for that bom project the deploy-plugin has its version not explicitly configured and for maven 3.8.6 2.7 is the default binding's version.
But the new parameter is only supported since maven-deploy-plugin version 2.8

In the latest Maven 3.9.2 release the default version of the maven-deploy-plugin is set to 3.1.0, which would be used in this case and work with the new format. Therefore I suggest to update to the latest Maven version.

When searching the Xtext repo for usages of the deploy-plugin I stumbled upon (the org.eclipse.xtext.maven.parent has a similar construct)

<!-- See https://jira.codehaus.org/browse/MINSTALL-102 -->
<plugin>
<artifactId>maven-enforcer-plugin</artifactId>
<configuration>
<bannedPlugins>
<excludes>
<exclude>org.apache.maven.plugins:maven-install-plugin</exclude>
<exclude>org.apache.maven.plugins:maven-deploy-plugin</exclude>
</excludes>
<includes>
<include>org.apache.maven.plugins:maven-install-plugin:2.5.2</include>
<include>org.apache.maven.plugins:maven-deploy-plugin:2.8.2</include>
</includes>
</bannedPlugins>
</configuration>
</plugin>

I wonder what the purpose of that part is? Is it to test that the deploy-plugin is really pinned to version 2.8.2 as defined a few lines below? Is the reason why the latest release cannot be used still valid? The added link is unfortunately dead.
Is it because of deployatend is used, which according to its doc was added in 2.8?
Furthermore it looks like the enforcer-plugin configuration is wrong, it has no rules element:
https://maven.apache.org/enforcer/enforcer-rules/bannedPlugins.html

If that's correct, I suggest to just remove the enforcer-plugin call here and would inherit the m-deploy/install-plugin version from the xtext-parent and do the same for org.eclipse.xtext.maven.parent).

The root pom.xml defines the version of maven-deploy-plugin as 3.0.0 and for the maven-install-plugin as 3.1.0.

@LorenzoBettini
Copy link
Contributor

Unfortunately, the BOM has no relation with the parent POM, so, for now, we should replicate a few plugin versions/configurations in the BOM, or things like the ones you described happen. This is required until a relationship is established between the BOM and the parent, but I still have to work on that.

Updating to Maven 3.9.2 is to be postponed due to the problems we'd have in GitHub Actions for macOS.

In general, anyway, it's best to pin versions of standard plugins (IIRC, Maven 4 will enforce that or the build will fail), and not rely on the ones provided by the current version of Maven.

Maybe the enforcer part could be removed now. I don't know why it's there, but it looks like it's due to old issues.

Currently, "deploy at end" is not used.

I'll try to address these issues in another PR.

@LorenzoBettini
Copy link
Contributor

I now reduced the PR to the changes you requested.

Please note the deployment of milestone and release to maven central needs manual closing of staged repo

If you use the nexus-maven-plugins to deploy to OSSRH you can set the autoReleaseAfterClose property to true to let the plugin close the repo for you. But I assume that does not apply for nightly builds?

To be able to test releases and milestones, we removed the use of nexus staging plugin.

When you deploy snapshots there's no additional staging going on: snapshots will be deployed immediately.

@LorenzoBettini
Copy link
Contributor

Rather than doing nightly builds at a fixed schedule, should we do continuous deployment whenever something's merged into main, @cdietrich ?
Not necessarily in the scope of this ticket, though @HannesWell

For me this is a good proposal. We do that in m2e and it works quite well and is actually not too complicated: https://github.com/eclipse-m2e/m2e-core/blob/7b340f774365df15715a92f87ca81bad2bc8a0e0/Jenkinsfile#L68-L97

But at least in m2e we have the deployment script in the Jenkinsfile in contrast to Xtext where the deployment is specified pom.xml of the repos (if I have not misunderstood it). So depending on how that should be done it might require some rework.

Performing the deployment on each push would probably make the question about merging the nightly- and release-deploy pipeline obsolete, since it probably has to be done as part of the 'main' pipeline then.

I really want to keep the deployment logic in POM only and avoid additional scripts for the deployment.

If we want to deploy (p2 and Maven snapshots) when something is pushed to main we can add a stage with the when condition as in https://github.com/eclipse-m2e/m2e-core/blob/7b340f774365df15715a92f87ca81bad2bc8a0e0/Jenkinsfile#L68-L97 but we'll have to run a build again (skipping tests and signing). In fact, we cannot sign when running tests as we saw in the past. It's not a big deal anyway to run the build again; it will be much faster since we'll skip tests.

@LorenzoBettini
Copy link
Contributor

Or even better: in the additional stage that should run only on main we simply trigger the current nightly deploy job

@LorenzoBettini
Copy link
Contributor

@HannesWell please cherry pick this commit 0698ae1 on this PR; it should fix the problem.

@LorenzoBettini
Copy link
Contributor

@HannesWell that commit is now on master. I'm retriggering the job for your PR, which now should work.

@HannesWell
Copy link
Member Author

HannesWell commented May 17, 2023

@HannesWell that commit is now on master. I'm retriggering the job for your PR, which now should work.

Thanks @LorenzoBettini and sorry for the late action, I didn't have access to the right computer over the day.

Just rebased the branch on top of main, since your re-trigger seems to not have been merged with the main in Jenkins.

@HannesWell
Copy link
Member Author

In general, anyway, it's best to pin versions of standard plugins (IIRC, Maven 4 will enforce that or the build will fail), and not rely on the ones provided by the current version of Maven.

Yes that's right, pinning is even better. Btw. there are new versions available.
You don't have dependabot enabled for Xtext, have you?

@HannesWell
Copy link
Member Author

What do you think about moving the gpg keyring setup into a script and reference it from the nightly-deploy and milestone-deploy Jenkinsfile? This would avoid a duplicated definition.

sh 'gpg --batch --import "${KEYRING}"'
sh 'for fpr in $(gpg --list-keys --with-colons | awk -F: \'/fpr:/ {print $10}\' | sort -u); do echo -e "5\ny\n" | gpg --batch --command-fd 0 --expert --edit-key ${fpr} trust; done'

sh 'gpg --batch --import "${KEYRING}"'
sh 'for fpr in $(gpg --list-keys --with-colons | awk -F: \'/fpr:/ {print $10}\' | sort -u); do echo -e "5\ny\n" | gpg --batch --command-fd 0 --expert --edit-key ${fpr} trust; done'

@cdietrich cdietrich modified the milestones: Release_2.31, Release_2.32 May 29, 2023
@cdietrich cdietrich modified the milestones: Release_2.32, Release_2.33 Aug 27, 2023
@cdietrich cdietrich modified the milestones: Release_2.33, Release_2.34 Nov 17, 2023
@cdietrich cdietrich modified the milestones: Release_2.34, Release_2.35 Feb 27, 2024
@cdietrich cdietrich modified the milestones: Release_2.35, Release_2.36 May 23, 2024
@HannesWell
Copy link
Member Author

What do you think about moving the gpg keyring setup into a script and reference it from the nightly-deploy and milestone-deploy Jenkinsfile? This would avoid a duplicated definition.

@LorenzoBettini is that or this PR in general of interest or should it be abandoned?

Copy link

Test Results

  6 460 files  ±0    6 460 suites  ±0   3h 2m 45s ⏱️ - 9m 30s
 43 240 tests ±0   42 657 ✅ ±0    583 💤 ±0   0 ❌ ±0 
170 102 runs  +4  167 736 ✅ ±0  2 354 💤 ±0  12 ❌ +4 

Results for commit 8381ac8. ± Comparison against base commit 6c31185.

@LorenzoBettini
Copy link
Contributor

What do you think about moving the gpg keyring setup into a script and reference it from the nightly-deploy and milestone-deploy Jenkinsfile? This would avoid a duplicated definition.

@LorenzoBettini is that or this PR in general of interest or should it be abandoned?

@HannesWell maybe moving the gpg stuff into a separate script could be useful.

@cdietrich cdietrich modified the milestones: Release_2.36, Release_2.37 Aug 25, 2024
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