-
Notifications
You must be signed in to change notification settings - Fork 72
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
base: master
Are you sure you want to change the base?
Add workflow for automated version increments in pull-requests #2352
Conversation
eclipse-platform-parent/pom.xml
Outdated
<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> |
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.
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.
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.
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.
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.
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.
# 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) |
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 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? |
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'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?
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 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.
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.
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.
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: 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: 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 |
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'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
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'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
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> |
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.
Why is this removed?
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.
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.
3585eec
to
333a27f
Compare
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. 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. Having separated builds also helps to separate concerns and avoids that we collect everything in the |
333a27f
to
975f59a
Compare
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:
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'stycho-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.