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

Add workflow for automated version increments in pull-requests #2352

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Sep 16, 2024

If an Eclipse Plugin project is changed the first time in a development cycle build failures due to missing version bumps are very common, for example like in the following case:

 [ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.8:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.pde.api.tools.tests:
Only qualifier changed for (org.eclipse.pde.api.tools.tests/1.3.600.v20240905-2015). Expected to have bigger x.y.z than what is available in baseline (1.3.600.v20240806-1811) -> [Help 1]

This PR adds a workflow to automatically detect the need for such service version increments whenever a PR is created or updated, using a fast running Maven build that invokes the tycho-p2-extras-plugin:compare-version-with-baselines goal. The required version bumps are then applied by invoking Tycho's tycho-versions-plugin:bump-versions goal. The result is committed with the expected message and pushed to the PR's branch using a configurable Eclipse bot account.
Furthermore the configured bot also adds a comment to the affected PR to inform about what happened, to list the files that were changed and to provide a git-patch of the change.

In order to make this possible a personal access token] (PAT) with scope public_repo:

With this all set up and running, one does not have to apply version bumps due to qualifier differences anymore and we also don't have to explain it to new users because everything just happens automatically.

An example of the current state can be seen in my PDE fork where I configured my own main account to act as bot-account:

This is the major implementation part of https://gitlab.eclipse.org/eclipse-wg/ide-wg/community/-/issues/56. The other parts are to set up the repositories where the new workflows should be called to use the described functionality.
I'll start with the PDE repository and also use it as a verification that this really works. Afterwards I'll set this up for all repositories that are interested.

The next steps are to ask the EF infra team to set up the corresponding tokens described above.

@HannesWell HannesWell changed the title Add workflow for automated version increments for pull-requests Add workflow for automated version increments in pull-requests Sep 16, 2024
Comment on lines 929 to 987
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-antrun-plugin</artifactId>
<executions>
<execution>
<id>resolve-version-qualifier</id>
<goals>
<goal>run</goal>
</goals>
<phase>generate-sources</phase>
<configuration>
<exportAntProperties>true</exportAntProperties>
<target>
<!-- See https://stackoverflow.com/a/53227117 and http://ant-contrib.sourceforge.net/tasks/tasks/index.html -->
<taskdef resource="net/sf/antcontrib/antlib.xml"/>
<if>
<or>
<equals arg1="${project.packaging}" arg2="eclipse-plugin"/>
<equals arg1="${project.packaging}" arg2="eclipse-test-plugin"/>
</or>
<then>
<copy file="META-INF/MANIFEST.MF" tofile="${project.build.directory}/MANIFEST.MF"/>
<manifest file="${project.build.directory}/MANIFEST.MF" mode="update">
<attribute name="Bundle-Version" value="${qualifiedVersion}"/>
</manifest>
<property name="version.check.skip" value="false"/>
</then>
<elseif>
<equals arg1="${project.packaging}" arg2="eclipse-feature"/>
<then>
<copy file="feature.xml" tofile="${project.build.directory}/feature.xml"/>
<replace file="${project.build.directory}/feature.xml"
token='version="${unqualifiedVersion}.qualifier"' value='version="${qualifiedVersion}"'/>
<manifest file="${project.build.directory}/MANIFEST.MF"/>
<property name="version.check.skip" value="false"/>
</then>
</elseif>
<else>
<property name="version.check.skip" value="true"/>
<manifest file="${project.build.directory}/MANIFEST.MF"/>
<echo message="Configure to skip version checks"/>
</else>
</if>
</target>
</configuration>
</execution>
</executions>
<dependencies>
<dependency>
<groupId>ant-contrib</groupId>
<artifactId>ant-contrib</artifactId>
<version>1.0b3</version>
<exclusions>
<exclusion>
<groupId>ant</groupId>
<artifactId>ant</artifactId>
</exclusion>
</exclusions>
</dependency>
</dependencies>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<executions>
<execution>
<id>pack-pseudo-jar</id>
<goals>
<goal>jar</goal>
</goals>
<phase>generate-sources</phase>
<configuration>
<classesDirectory>${project.build.directory}</classesDirectory>
<includes>
<include>feature.xml</include>
<include>MANIFEST.MF</include>
</includes>
<archive>
<manifestFile>${project.build.directory}/MANIFEST.MF</manifestFile>
</archive>
</configuration>
</execution>
</executions>
</plugin>
Copy link
Member Author

Choose a reason for hiding this comment

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

When eclipse-tycho/tycho#4279 and eclipse-tycho/tycho#4280 respectively their back-ports are available this can be much simpler and specially can work without explicit file manipulations.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of packing a "pseudo-jar" here? Usually skipping tests (and maybe using multiple threads) makes the packing fast enough, trying to replicate the packing to safe a few seconds does not seems very useful given that we perform non trivial task there.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was just a work-around because the two tycho changes mentioned above were not yet available.
Now it's just an invocation of two tycho-plugins configured to do the minimally necessary, plus an antrun invocation to set a few properties based on the project's packaging type.

Of course it would be even very nice if just the tycho-p2-extras-plugin:compare-version-with-baselines goal could be invoked without a preceding tycho-p2-plugin and tycho-packaging-plugin, but that seemed to be more work (although it was also more work than expected to strip that all down).

Usually skipping tests (and maybe using multiple threads) makes the packing fast enough, trying to replicate the packing to safe a few seconds does not seems very useful given that we perform non trivial task there.

This not only saves a few seconds. With the current set up a build takes in the PDE example about 45s, while a full build even if executed in parallel and without tests takes about 5min (on my machine). If it is considers that in case of required version bumps at least one more build is required this is already a difference of two vs. 10min response time.
In the PDE example presented above, even three build runs are necessary so the response time that's currently between three/four minutes (the workflow has an overhead) would go up to ~15min. Which is also added to the overall build time.
And I think the response time should be fast.

If this does not work anymore we can still go back to mvn clean verify -DskipTests -T 1C -Dcompare-version-with-baselines.skip=false or make tycho-p2-extras-plugin:compare-version-with-baselines work with only a qualifier being computed before.

Comment on lines +117 to +124
# TODO: Add an FAQ about version bumps?
# - Why are the version bumps necessary at all?
# According to the rules of semantic versioning at least the service version of a bundle should be increased if anything changes between releases.
# - Why is the service version bumped by +100
# Allows service version increments on maintanance branches that stay semnatically smaller than the next releases version
# - Why a separate commit?
# Easier to identify in Git history and the actual change is easier to revert (version must not be reverted)
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 intend to add an folded FAQ to the comment added by the bot so that especially newcommers can understand better why these version bumps are necessary and why they are done this way.

The list is currently a brief summary, but if you think questions are missing, please let me know.

# Allows service version increments on maintanance branches that stay semnatically smaller than the next releases version
# - Why a separate commit?
# Easier to identify in Git history and the actual change is easier to revert (version must not be reverted)
# TODO: make comment updatable?
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'm contemplating to make the comment added by the bot updatable, to avoid new comments being added after each (force-)push, if the version bump isn't fetched and pushed as well.
But this also means that updates are less visible.
It can for example also happen that due to an update no version bumps are necessary anymore (but the workflow doesn't do anything in this case anyways).

What's your preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it must not be configurable, but most confess it a bit unfortunate that we optimize for backports (happen very seldom) and reverts (should actually never happen), anyways a link to the FAQ would be suitable enough for me we don't need a summary replicated on each PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was this intended for #2352 (comment)?

But sure providing the FAQ in the wiki and linking it is of course also a good solution. It would be folded, just like the git-patch, but with a link one can always read the latest version.
I can implement it that way.

but most confess it a bit unfortunate that we optimize for backports (happen very seldom) and reverts (should actually never happen)

I wouldn't say we optimize it, but with the workflow it also just technically simpler. And just as a side-note: since these things happen seldomly it's actually better to have less things to consider because otherwise mistakes are more likely.

@laeubi
Copy link
Contributor

laeubi commented Sep 17, 2024

Instead of having an own build I think one should adopt to what other workflows do (e.g. the junit-publishing workflow), where they work on the result of a previous workflow. e.g if one looks here:

grafik

there is an upload event file step already that contains all the meta-data of the pull request.

Later on, unit test results are uploaded:
grafik

now the Publish test results workflow can read them and act upon these.

The only thing needed is then that we probably upload additional information e.g required version bumps can be written to a file in whatever format suitable, maybe even as a patch file by Tycho and don't need to trigger additional builds at all and we make sure we always reuse what Tycho already offers, this could also probably require less permissions and make the workflow easier to understand.

if: steps.search-patch.outputs.result
with:
ref: '${{ github.event.workflow_run.head_sha }}'
persist-credentials: true # (Explicit) opt-in for persisting the bot's PAT for authentication when pushing below
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is something we want here if it runs on PRs submitted by users from this comment:

because it means any workflow using actions/checkout basically leaks the token to any process/action in that workflow which can just read it from .git/config.

Can't we use

https://github.com/marketplace/actions/create-pull-request

instead that already supports all the handling of tokens? According to the docs one can also prepare the commit manually:
https://github.com/marketplace/actions/create-pull-request#create-your-own-commits

one also can push to forked repositories:
https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#push-pull-request-branches-to-a-fork

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'm not sure this is something we want here if it runs on PRs submitted by users from this comment:

because it means any workflow using actions/checkout basically leaks the token to any process/action in that workflow which can just read it from .git/config.

Yes this is something I also wanted to discuss, but in general it should not be very dangerous because this happens only in the workflow triggered upon the completion of the first workflow that runs the build to determine the required changes. And that second workflow only applies the git-patch of these changes and pushes it.
So no changes supplied from external persons should be executed in any form.

Can't we use

https://github.com/marketplace/actions/create-pull-request

Unfortunately not because this workflow pushes an extra commit to an already existing PR and does not aim to create a new one (as far as I understand that's not possible with the action you mention?).
However there are other actions just for pushing, but in the end they just push via https using basic-auth:
https://github.com/ad-m/github-push-action/blob/77c5b412c50b723d2a4fbc6d71fb5723bcd439aa/start.sh#L52

I have now changed this part to do the same when pushing. This way the token is only supplied to the run step as an environment variable. Initially I implemented it this way, but changed it to persisted credentials since it seems a bit simpler and as described above, the second workflow should actually not be critical.
If you think it's necessary I can also move the push part to a separate run step to further narrow the availability of the token.

@@ -768,33 +780,6 @@
</repository>
</repositories>
</profile>
<profile>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

With this new workflow to apply and push required version increments automatically to a PR, this seems not to be necessary anymore since the new workflow will handle this.
So removing this and the version-bump part in the updateRelease.yml reduce amount of configuration and avoids that duplicated configuration goes out of sync (e.g. if we want other version-bump deltas in the future).

But I moved all removals to separate commit in this PR to make this more clear.

@HannesWell
Copy link
Member Author

HannesWell commented Sep 18, 2024

Instead of having an own build I think one should adopt to what other workflows do (e.g. the junit-publishing workflow), where they work on the result of a previous workflow. e.g if one looks here:
[...]
The only thing needed is then that we probably upload additional information e.g required version bumps can be written to a file in whatever format suitable, maybe even as a patch file by Tycho and don't need to trigger additional builds at all and we make sure we always reuse what Tycho already offers, this could also probably require less permissions and make the workflow easier to understand.

First of all I started out simple and wanted to keep it simple, but unfortunately it turned out that if you want to push to a PR-branch in a fork from a GitHub workflow, things become complicated. I tried out a lot and tried to keep it as simple as possible.
The main problems are that first the GITHUB_TOKEN has no permission to push to forks, even if I grant write-all permissions, so a PAT is necessary for pushing. And second, workflows triggered by the pull_request event from a fork of a non-committer (i.e. an account without write permissions) don't have access to the secret store of a repo.
Therefore a second workflow is necessary that runs upon completion of the pull_request even triggered workflow, similar to the workflow to publish test results (although it's a bit different there because that workflow only adds a comment, but that's also not possible for a workflow triggered by a PR from a fork).

It would be possible to have the version bumps applied as part of the regular verification builds, but that would only save us one extra git-checkout step plus the configuration of a JDK and Maven as well as that we have a separate file. But all other steps and inputs etc. done in the first workflow still would have to be done and the second workflow would still be necessary at it is.
This moderate simplification of the configuration would come with the draw-back that each iteration would take the full time of the verification build. In the PDE example this would mean ~30min per iteration instead of 45s. In the linked example this would mean a total wait-time of roughly 1.5h until the PR is completely built instead of ~3-4min extra time (see #2352 (comment)).
For me this justifies the current setup.

Having separated builds also helps to separate concerns and avoids that we collect everything in the mavenBuild.yml.

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.

2 participants