gazebo-tooling / release-tools

8 stars 9 forks source link

PR jobs are not being triggered for branches that target other PRs #1044

Open azeey opened 11 months ago

azeey commented 11 months ago

From time to time, we break down a task into a series of smaller PRs and target each PR to the branch of the next PR. We also sometimes create PRs as suggestions for changes in another PR when comments are difficult to convey all the changes required by the reviewer. These workflows seem to be currently broken because the pr_any jobs have a branch whitelist that includes only the stable branches in each repo. For example, https://build.osrfoundation.org/job/sdformat-ci-pr_any-focal-amd64/ is configured to trigger only for PRs targetting sdf12 and sdf13, so it didn't run for https://github.com/gazebosim/sdformat/pull/1341 which targets https://github.com/gazebosim/sdformat/pull/1339 which targets sdf13.

azeey commented 11 months ago

@j-rivero Have we considered having a .jenkins file in each library to keep this kind of configuration? It might be difficult to know the base branch of a PR otherwise.

j-rivero commented 11 months ago

These workflows seem to be currently broken because the pr_any jobs have a branch whitelist that includes only the stable branches in each repo.

Ugh. Yep, the new design which specifies in yaml file which are the target platforms for the corresponding branches can not work with this model of PR pointing to other places than branches listed in the yaml file.

@j-rivero Have we considered having a .jenkins file in each library to keep this kind of configuration? It might be difficult to know the base branch of a PR otherwise.

Yep, in fact we were doing something similar with the old -ubuntu-auto jobs some time ago by getting the DISTRO from a value in a configuration file inside the repo. Having the target platforms listed in a single place has a lot of value as a single point of truth compared to maintain a CI info on each of the simulation repositories. That came with the cost of limiting use cases like this one.

I'll discuss with the infra team the topic to see if there is any solution that does not imply having to store the value in 10 different repositories across 3-4 branches on it.

Meantime I think that the workaround is going to be to manually call the CI jobs and report results in the comments.

j-rivero commented 11 months ago

I'll discuss with the infra team the topic to see if there is any solution that does not imply having to store the value in 10 different repositories across 3-4 branches on it.

@nuclearsandwich suggested a possible solution: we can use the branch name to indicate the final base branch that you are targeting with PRs not directly targeting stable branches. Particularly we can include a prefix in the form: ci/${target_final_stable_branch}/${branch_name}

Example:

For the workflow: 
sdf13 <- pr_base <- pr_part1 <- pr_part2

We can create branch names:
sdf13 <- ci/sdf13/pr_base  <- ci/sdf13/pr_part1 <- ci/sdf13/pr_part2

Should be trivial to make the current -pr_any- jobs for stable branches to watch ci/${target_final_stable_branch}.

azeey commented 10 months ago

From our VC conversation, this is not convenient for developers because it's easy to forget that we have to follow the specific branch naming pattern when creating a PR train or creating a suggestion PR when reviewing another PR. We discussed the possibility of parsing the major version from CMakelists to figure out the base branch, but that may not be possible if the tool that generates the should not be reading the code for various reasons. @j-rivero can you verify this?

Another option is to determine the major version by looking at the last release tag that can be reached from the base branch (i.e using git describe and some regex).

j-rivero commented 10 months ago

We discussed the possibility of parsing the major version from CMakelists to figure out the base branch, but that may not be possible if the tool that generates the should not be reading the code for various reasons. @j-rivero can you verify this?

There is a fundamental problem assuming that parsing the CMakeLists.txt can tell us the final target branch and the CI platform with it. We would need to assume that branches matches the project names in CMakeLists.txt which is something we have been doing but would be great to stop doing since it is fragile.

There is also a problem with the implementation: we have a chicken and egg problem here since we are launching CI on a given platform since we have the platform-CI job watching that branch specifically (i.e: the -ubuntu-jammy job is being triggered by the PRs against gz-sim8). So we would need to introduce a "pre-step" on a "neutral platform" that calculates which is going to be the CI platform.

Another option is to determine the major version by looking at the last release tag that can be reached from the base branch (i.e using git describe and some regex).

The chicken-and-egg problem here appears again, these git operations needs to happen somewhere and after them we would need to call the right platform job.

Nothing impossible, just not trivial.

j-rivero commented 10 months ago

@azeey proposed that we tried to include the branch in the PR summary. That should be possible using that feature of the github plugin. I'll give it a try.

j-rivero commented 9 months ago

That should be possible using that feature of the github plugin. I'll give it a try.

Looking deeply into this, I think that we found the configuration to skip the CI inserting a message in the commit or body but did not find a way of setting up triggering based on a regexp on the body of the PR message.

Alternatively we could potentially react on PR tags, so we could have something like force_ci. This is restricted byt:

  1. Current pr_any jobs are using whiteListed branches. If we add the tags configuration to it, it will operate as a new resetriction on top of the whiteListed branches so it won't work. The jobs can not have two alternative configurations for GitHubPR plugin.
  2. As consequence of 1. new jobs needs to be added to the DSL to handle this use case. One per repository + one per platform (Ubuntu distros + Win + Brew). Not difficult in DSL but adds overhead.
  3. The label should somehow have the stable branch information or at least the Linux distro to use to have the metadata related to platform