feat: Add pull request support to SCM generator #469
Conversation
fardin01
force-pushed
the
466
branch
13 times, most recently
from
fcc7a46 to
3083c51
Compare
|
Waiting for #472 to be merged. |
fardin01
force-pushed
the
466
branch
3 times, most recently
from
52bb958 to
7002822
Compare
fardin01
changed the title
fardin01
force-pushed
the
466
branch
12 times, most recently
from
740f7c0 to
ea586e4
Compare
fardin01
force-pushed
the
466
branch
23 times, most recently
from
51036c5 to
bc4dec3
Compare
| // Scan all branches instead of just the default branch. | ||
| AllBranches bool `json:"allBranches,omitempty"` | ||
| // Scan all pull requests | ||
| AllPullRequests bool `json:"allPullRequests,omitempty"` |
There was a problem hiding this comment.
There should be documentation on what to expect the generator to return if both allBranches and allPullRequests are true.
| allPullRequests: true, | ||
| url: "git@github.com:argoproj/applicationset.git", | ||
| branches: []string{"pr-1", "pr-2"}, | ||
| }, |
There was a problem hiding this comment.
Probably need a test case for both allPullRequests and allBranches being true, whatever the behavior might be in that case.
| name: "all pull requests", | ||
| allPullRequests: true, | ||
| url: "git@github.com:argoproj/applicationset.git", | ||
| branches: []string{"pr-1", "pr-2"}, |
There was a problem hiding this comment.
I'm not sure what this test means... if we're listing pull requests, what is the meaning of checking the results for two branches named pr-1 and pr-2?
There was a problem hiding this comment.
Okay, I misunderstood what this test actually does. So it gets real data from applicationset Github repo and tests against that. In that case, in order to test for pull requests, we'll have to open a "dummy" PR and keep it always open for this purpose. Does that sound alright?
There was a problem hiding this comment.
On second thought, we can do that with allPullRequests set to true and a filter to look for a specific PR. We can't realistically list all the PRs and check if their branch is what we expect them to be. We could set allPullRequests to true and check for the response code. How does that sound?
| return false, nil | ||
| } | ||
|
|
||
| if filter.PullRequestLabelMatch != nil { |
There was a problem hiding this comment.
Since this performs the same check as filter.LabelMatch, why not just use that filter?
There was a problem hiding this comment.
With adding the PullRequest field to the Repository struct, we can keep this and range through repo.PullRequest.Labels instead of repo.Labels.
#469 (comment)
There was a problem hiding this comment.
Yep, I think that's the way to go.
Will need to consider how to treat a pull-request filter placed on an SCM generator where pull request fetching isn't enabled. Ignore, log error, etc.
| @@ -342,6 +346,10 @@ type SCMProviderGeneratorFilter struct { | |||
| LabelMatch *string `json:"labelMatch,omitempty"` | |||
| // A regex which must match the branch name. | |||
| BranchMatch *string `json:"branchMatch,omitempty"` | |||
| // A regex which must match the pull request tile. | |||
| PullRequestBranchMatch *string `json:"pullRequestTitleMatch,omitempty"` | |||
There was a problem hiding this comment.
Should there be two filters, one to match source branches, and the other to match target branches?
There was a problem hiding this comment.
That's an interesting point! I can't recall ever seeing a source branch being merged into a non-default target branch (main, master etc.). In addition, usually, all source branches get merged into one single target branch (otherwise you got a very messy repo 😄 ) which makes filtering the target branch pointless. So IMHO this feature would only support a "corner case". What do you think?
There was a problem hiding this comment.
Consider this use case: I want to spin up test environments for each PR against main. Developer A opens a PR from feature/whatever to main. Developer B has suggestions but instead of writing them all out, just opens a PR from feature/whatever-review to feature/whatever.
I think this is common enough to justify giving the AppSet the designer to avoid creating an App for that second PR.
There was a problem hiding this comment.
But if it ends up being a lot of work, we could certainly leave it for a future enhancement.
…enerator can create ArgoCD apps for PRs as well. Fixes argoproj#466 Signed-off-by: Fardin Khanjani <fardin.khanjani@tradeshift.com>
|
@rishabh625 Please feel free to move this PR over to argocd repo. I should have some time to get back at this pretty soon. Thanks :) |
This commit adds pull request support to SCM generator so the generator
can create ArgoCD apps for PRs as well.
Fixes #466
Signed-off-by: Fardin Khanjani fardin.khanjani@tradeshift.com