SCM Provider with Pull Requests

[fardin01]

Problem

Both the SCM Provider generator and Pull Request generator are very good tools for different and valid use cases, but the limitation is that they cannot be used together in a Matrix generator:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: myapp
spec:
  generators:
    - matrix:
        generators:
        - scmProvider:
            github:
              organization: myorg
        - pullRequest:
            github:
              owner: myorg
              repo: < Cannot do this here! >
  template:
   ....

We are trying to adopt/migrate to ArgoCD and this is sort of a major blocker. Our testing ecosystem depends on engineers being able to deploy PRs to validate features. According to Pull Request generator docs, "this [the genrator] fits well with the style of building a test environment when you create a pull request". However, when using ApplicationSets, we cannot really have one ApplicationSet with SCM generator and one with Pull Request generator.

Proposed solution

Add an extra attribute to SCM generator:

  generators:
  - scmProvider:
      github:
        organization: myorg
        allBranches: false
        allPullRequests: true

Similar to allBranches, allPullRequests would create a new ArgoCD app for the open PRs, and could also have a PRMatch filter.

So it would be like embedding the Pull Request generator into the SCM generator.

Any feedback/comments would be appreciated.

[fardin01]

A better alternative: Change the Pull Request generator to accept a pattern/regex for repo attribute, and if left empty it should find all the repos. Using this in conjunction with SCM generator will most likely hit the rate limits within seconds, so it must be very well optimized.

[fardin01]

The above alternative is probably a good enhancement to the Pull Request generator in general. But embedding the Pull Request generator into the SCM generator has the added benefit of using the SCM generator's filters and listing pull requests only for the filtered repos.

[fardin01]

@jgwest Would love your thoughts before I start working on this :)

[crenshaw-dev]

@fardin01 how would you feel about adding templating support to the second generator in a matrix generator? Basically pass each variable set produced by the first generator to the second generator and perform substitution before running the generator.

Something like this:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: myapp
spec:
  generators:
    - matrix:
        generators:
        - scmProvider:
            github:
              organization: myorg
        - pullRequest:
            github:
              owner: myorg
              repo: "{{repoURL}}"
  template:
   ....

[fardin01]

@fardin01 how would you feel about adding templating support to the second generator in a matrix generator? Basically pass each variable set produced by the first generator to the second generator and perform substitution before running the generator.

Something like this:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: myapp
spec:
  generators:
    - matrix:
        generators:
        - scmProvider:
            github:
              organization: myorg
        - pullRequest:
            github:
              owner: myorg
              repo: "{{repoURL}}"
  template:
   ....

@crenshaw-dev I think templating support in the second generator would be an excellent idea generally, but not so much in this specific use-case, because the matrix generator (at least at this moment) supports only two child generators. What if someone wants to use the SCM Provider (with PR support) and e.g. cluster generator (which probably is a common real-world scenario)? If the SCM Provider supports PRs by itself, then it would become more useful in a matrix generator. What do you think?

I'd also appreciate it if you could take a look at the PR for this issue.

[crenshaw-dev]

@fardin01 will review!

I think there might be a couple possible fixes for the matrix generator limitations:

1. use a nested matrix generator, providing up to 3 generators
2. add support for N generators in a matrix generator

But those are certainly more involved than your fix. Since your change just adds a field that we could later deprecate to go another direction, I think it's probably quite safe.