-
Notifications
You must be signed in to change notification settings - Fork 6.7k
feat(appset): add time-based filtering in PR generator #25803
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
base: master
Are you sure you want to change the base?
feat(appset): add time-based filtering in PR generator #25803
Conversation
❗ Preview Environment stop on Bunnyshell failedSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
e40c8a0 to
e844c46
Compare
| Labels: getGithubPRLabelNames(pull.Labels), | ||
| Author: *pull.User.Login, | ||
| CreatedAt: pull.CreatedAt.Time, | ||
| UpdatedAt: pull.UpdatedAt.Time, |
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.
are there no tests where we can add these fields? I see that we have tests for other providers that you edited to add the timestamp field but not for GitHub.
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.
That is correct. I've just added additional test cases for Github PR generator service.
nitishfy
left a comment
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.
Please fix the conflicts.
b4baa65 to
3d4e4bd
Compare
Conflicts resolved @nitishfy |
ppapapetrou76
left a comment
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.
Consider adding a small log/event when a PR is skipped due to filtering (so users can debug why apps were not created).
Please make sure time comparisons use the time format Since/UTC properly to avoid DST/timezone surprises.
| updatedTime = "2015-10-15T17:38:55.491628+00:00" | ||
| ) | ||
|
|
||
| func parseBitbucketCloudTimeFromString(t string) time.Time { |
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.
this function already exists above in another test
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've removed this function from gitea_test.go and reused this one for both gitea and bitbucket_cloud
| if filter.TitleMatch != nil && !filter.TitleMatch.MatchString(pullRequest.Title) { | ||
| return false | ||
| } | ||
| if filter.CreatedWithin != nil && pullRequest.CreatedAt.Before(time.Now().Add(-*filter.CreatedWithin)) { |
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.
createdWithin takes precedence over updatedWithin — please make this explicit in both code comments and user docs
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've mentioned this in the docs for createdWithin filter.
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've mentioned this in the docs for
createdWithinfilter.
Can we also add a note in the CLI argument description. Most users will check the CLI output first rather than the dcos.
WDYT?
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 thinking if this comments and documentation updates are needed since this is pretty much the behavior for all conditions if they are provided within a single filter like this:
- branchMatch: "test-*"
createdWithin: "12h"
updatedWithin: "6h"In this case filter wants all conditions to be true so if any of these conditions fail, PR will be filtered out and there will be no difference if 1st, 2nd or 3rd condition failed and any other passed.
I think it is my false assumption that there is "precedence" while I was initially working on this PR and I apologize, I will update PR description to remove that.
That is already mentioned in the docs here.
6e035dd to
7193d81
Compare
@ppapapetrou76 That will be useful for debugging. I've added log that will show properties of pull request and filters for each PR that is filtered out during appset reconciliation. Also, I've updated tests and date parsing methods for SCM providers to use UTC. |
|
Also, with this PR ApplicationSet CRD has issues when manifests are installed without |
| } | ||
| if filter.UpdatedWithin != nil && pullRequest.UpdatedAt.Before(time.Now().UTC().Add(-*filter.UpdatedWithin)) { | ||
| return false | ||
| } |
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.
we can make this code a little bit more readable by moving these lines in PullRequest struct methods ( interface.go ) file
You can have something like
func (p PullRequest) IsCreatedWithin(t *time.Duration) bool {
return t != nil && p.CreatedAt.Before(time.Now().UTC().Add(-*t))
} Same for UpdatedWithin
WDYT?
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 agree, what do you think about moving all checks to methods?
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.
Yes but for now let's do the checks related to the context of this 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.
I've created methods for checking if PR is created or updated within specified time duration.
| if filter.CreatedWithin != nil { | ||
| createdWithin, err := time.ParseDuration(*filter.CreatedWithin) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing CreatedWithin duration %s: %w", *filter.CreatedWithin, err) | ||
| } | ||
| outFilter.CreatedWithin = &createdWithin | ||
| } | ||
| if filter.UpdatedWithin != nil { | ||
| updatedWithin, err := time.ParseDuration(*filter.UpdatedWithin) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing UpdatedWithin duration %s: %w", *filter.UpdatedWithin, err) | ||
| } | ||
| outFilter.UpdatedWithin = &updatedWithin | ||
| } |
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.
do you think we can remove duplication by adding a function?
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.
Yes, we might want to do that for other filters too?
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.
let's stick to time-based filter for now in this 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.
I've updated both filters to use function.
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
Signed-off-by: Marko Mirosavljev <[email protected]>
7193d81 to
55a39a1
Compare
Signed-off-by: Marko Mirosavljev <[email protected]>
Closes #25804
Summary
This PR adds 2 additional filters in applicationSet PR generator,
createdWithinandupdatedWithinwhich allow PR filtering based on time after they are created, or last time updated.Motivation
Implementation
createdAtandupdatedAtfield to pull request parameters - both parsed from values returned by SCM APIs and will also be available for templatingcreatedWithinandupdatedWithinfilters on applicationset CRD and updated filtering functions to parsetime.Durationfrom CRD while compiling and applying filters to applicationset.Manifests will look like this:
Checklist: