Support declaring PR dependencies in PR description

[christophebedard]

Closes #157

This allows declaring PR dependencies (PRs/MRs/branches) in a PR description, e.g.

Description of the changes.

Blah blah.

action-ros-ci-dependency : https://github.com/user/some-repo/pull/123
action-ros-ci-dependency : https://gitlab.com/user/other-repo/-/merge_requests/123
action-ros-ci-dependency : https://github.com/other-user/some-repo/tree/feature/some-branch
action-ros-ci-dependency : https://gitlab.com/other-user/other-repo/-/tree/feature/some-branch

but without the space before the colon (otherwise e2e tests for this PR will try to clone those fake repos).

If no dependencies are declared, this feature shouldn't affect anything. I did have to slightly modify the existing logic for this to work. Instead of piping curl to vcs import, we now download all .repos files into a specific directory using curl. If PR dependencies were declared, the .repos files are then modified to reflect that (and an extra file may be added if a dependency isn't in any of the downloaded files; see README). Finally, the repos are imported using vcs import.

Given the federated nature of ROS (and as someone who works on the only core ROS 2 repo hosted on GitLab 😆), I wanted to have this support both GitHub and GitLab PRs/MRs/branches (with a caveat; see README).

To provide some feedback, a list of links of detected dependencies is printed in the action output, e.g.:

or when there are no dependencies:

The feature is currently only tested using unit tests since end-to-end tests are a bit harder for this, but I have tested it extensively and it works great.

#157 mentioned handling cases where PRs/MRs are closed/merged, but I am putting that aside for now. The action will just fail in those cases (vcs import will fail since the PR/MR ref won't exist).

[codecov]

Codecov Report

Attention: Patch coverage is 71.07438% with 35 lines in your changes missing coverage. Please review.

Project coverage is 53.48%. Comparing base (377221a) to head (0fb6367).
Report is 212 commits behind head on master.

Files with missing lines	Patch %	Lines
src/action-ros-ci.ts	8.10%	34 Missing ⚠️
src/dependencies.ts	98.80%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #490       +/-   ##
===========================================
+ Coverage   36.00%   53.48%   +17.48%     
===========================================
  Files           1        2        +1     
  Lines         150      258      +108     
  Branches       29       49       +20     
===========================================
+ Hits           54      138       +84     
- Misses         96      120       +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[christophebedard]

@rotu @emersonknapp here is the solution I am proposing. Let me know what you think!

and sorry for the rather long delay!

[christophebedard]

@emersonknapp friendly ping

[christophebedard]

@emersonknapp friendly ping

[emersonknapp]

Thanks for the patience - it's taken a while to get to this.

I like the idea, and the interface looks perfect to me - the README makes it very clear and I don't disagree with anything there. See above high level comment - I think a little bit of restructuring will make this a lot easier/require a lot less code, let me know what you think about that

[emersonknapp]

High level - this logic would look a lot cleaner to me if we avoided mutating the input files entirely, in favor of a simple overlay scheme - pseudocode to show what i mean:

const prDependencies = dep.getPrDependencies(github.context.payload);
const dependenciesRepoPath = tempfile();
dep.writeDependenciesToReposFile(prDependencies, dependenciesRepoPath);

for (const vcsRepoFileUrl of vcsRepoFileUrlListResolved) {
  execBash(`curl ${vcsRepoFileUrl} | vcs import --force --recursive src/`)
}
execBash(`vcs import --force --recursive src/ < dependenciesRepoPath`);

The --force option already forces an overwrite of existing repositories in the path, so a modification doesn't make it any more logically correct - and this way I think we could get rid of at least a hundred lines of matching logic in dependencies.ts

Does this sound good, or am I missing some crucial point?

[christophebedard]

I don't think that works, because vcs import --force [--recursive] only overwrites repos (i.e. checks out another branch in our case) if directory paths match.

Here's a counter-example. Let's say we have this .repos file as a "base" file (i.e. element in vcsRepoFileUrlListResolved).

# original.repos, e.g. ros2.repos
repositories:
  ros-tracing/ros2_tracing:
    type: git
    url: 'https://gitlab.com/ros-tracing/ros2_tracing.git'
    version: master

and then this file gets generated from declared dependencies (dependenciesRepoPath):

repositories:
  ros2_tracing:
    type: git
    url: 'https://gitlab.com/ros-tracing/ros2_tracing.git'
    version: foxy

Result:

$ vcs import --force --recursive test < original.repos 
=== test/ros-tracing/ros2_tracing (git) ===
Cloning into '.'...

$ ll test 
total 0
drwxr-xr-x  3 chris  staff   96 28 Jan 17:09 .
drwxr-xr-x  8 chris  staff  256 28 Jan 17:09 ..
drwxr-xr-x  3 chris  staff   96 28 Jan 17:09 ros-tracing
$ vcs import --force --recursive test < dependencies.repos 
=== test/ros2_tracing (git) ===
Cloning into '.'...

$ ll test 
total 0
drwxr-xr-x   4 chris  staff  128 28 Jan 17:09 .
drwxr-xr-x   8 chris  staff  256 28 Jan 17:09 ..
drwxr-xr-x   3 chris  staff   96 28 Jan 17:09 ros-tracing
drwxr-xr-x  20 chris  staff  640 28 Jan 17:09 ros2_tracing

We just get a second repo, because the directory paths don't match. This is because we only provide URLs for dependencies; we don't provide a directory path to be used by dep.writeDependenciesToReposFile() in your example. We instead have to compare repo URLs, which is what I did. And I think providing anything more than a branch/MR/PR URL in PR descriptions would be rather cumbersome.

Now it's your turn to let me know if I'm missing some crucial point 😆

[emersonknapp]

hmm.. in practice, the ros2.repos base file has the repositories structured exclusively as

org/repo:
  type: git
  url: https://github.com/org/repo

We could easily force our output .repos file to match that convention. But - I think you're right that we can't depend on conventions for correctness though, that's going to lead to problems later - the desired directory structure isn't always src/org/repo.

This conversation however makes me realize a much larger problem with this feature as written - it won't work when PRs are coming from forks (unless I'm missing some crucial point) - the URL of the source repository is different than that of the target repository. This will mean that anybody without write access to the source project can't use this feature. I have solved this in practice for my script that kicks of Jenkins jobs by actually using github APIs to understand pull requests - here is the function, it should give the idea

def create_ci_gist(
    github_instance: Github,
    pulls: List[github.PullRequest.PullRequest],
    target_release: str
) -> github.Gist.Gist:
    """Create gist for the list of pull requests."""
    logger.info("Creating ros2.repos Gist for PRs")
    master_repos = fetch_repos(target_release)
    for github_pr in pulls:
        pr_ref = github_pr.head.ref
        pr_repo = github_pr.head.repo.full_name
        base_repo = github_pr.base.repo.full_name

        # Remove the existing repository from the list
        repo_entry = master_repos.pop(base_repo, None)
        if repo_entry:
            if repo_entry['type'] != 'git':
                panic('Chosen repository is not git-based')
        else:
            logger.warn(
                f'PR Repository "{pr_repo}" not found in ros2.repos, be aware that this is not '
                'part of the default build set and is not guaranteed to work.')

        # Add the same package from the PR's repository to the list
        master_repos[pr_repo] = {
            'type': 'git',
            'url': 'https://github.com/{}.git'.format(pr_repo),
            'version': pr_ref
        }

    yaml_out = yaml.dump({'repositories': master_repos}, default_flow_style=False)

    input_file = InputFileContent(content=yaml_out)
    gist = github_instance.get_user().create_gist(
        public=True,
        files={'ros2.repos': input_file},
        description='CI input for PR {}'.format(github_pr.url))
    return gist

the above as-is wouldn't help with gitlab or branches - but with a little extra effort, i think this could be written using the Javascript Octokit(github) and something like https://www.npmjs.com/package/gitlab.

I think if we took the above approach, we could either modify input .repos files, or create an overlay with the same paths for a given target repository - it seems this would be better than the complex regex logic, which is something I'm really trying to avoid having to maintain :)

[emersonknapp]

Oh - it seems like this might know how to handle forked versions - I'm just not understanding how that part works yet

[christophebedard]

But - I think you're right that we can't depend on conventions for correctness though, that's going to lead to problems later - the desired directory structure isn't always src/org/repo.

yeah!

This conversation however makes me realize a much larger problem with this feature as written - it won't work when PRs are coming from forks (unless I'm missing some crucial point) - the URL of the source repository is different than that of the target repository. This will mean that anybody without write access to the source project can't use this feature.

Oh - it seems like this might know how to handle forked versions - I'm just not understanding how that part works yet

I had to double check because I didn't remember 😆, but yeah it doesn't really understand or support forks. It only compares URLs.

I thought that perhaps this would still be useful (i.e. better than nothing), but if you think we should support forks "from day 1," I understand.

I'll work on adding fork awareness using the GitHub & GitLab APIs.

[christophebedard]

Another option could be to allow users to overwrite the list of .repos files (declared in their action-ros-ci inputs) by providing links in a PR description, like I did it for PRs/MRs/branches here. And we could also allow users to provide additional .repos files. This is basically what https://ci.ros2.org offers. It still requires you to manually create a .repos file (+ create a gist), but it should be way simpler, code-wise.

Thought I'd mention this as well.

[emersonknapp]

Another option could be to allow users to overwrite the list of .repos files (declared in their action-ros-ci inputs) by providing links in a PR description, like I did it for PRs/MRs/branches here. And we could also allow users to provide additional .repos files. This is basically what https://ci.ros2.org offers. It still requires you to manually create a .repos file (+ create a gist), but it should be way simpler, code-wise.

Thought I'd mention this as well.

I think I would like this feature - like you say it would be a lot simpler code and not much to nitpick. It could go in parallel (not in exclusion) to this feature to get something in more quickly - with a UI like:

action-ros-ci-overlay : https://gist.githubusercontent.com/emersonknapp/c78911c644a268d15405ea377138d831/raw/1164721eb3271e77660b295b6311f6ab03674bb1/ros2.repos

[christophebedard]

I think I would like this feature - like you say it would be a lot simpler code and not much to nitpick. It could go in parallel (not in exclusion) to this feature to get something in more quickly

Great! I'll work on it and create a separate PR.