Skip to content
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

EVG-13846 EVG-13946 Mainline commits query and graphql resolver. #4663

Merged
merged 50 commits into from
May 25, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
8184c81
Base mainline commits query
khelif96 Apr 28, 2021
d3ea4b6
Check if version is active and change return struct
khelif96 Apr 28, 2021
849a1ea
Merge branch 'main' into EVG-13846
khelif96 May 5, 2021
e1bca53
Fetch full version only when needed
khelif96 May 5, 2021
4a93c50
Remove unused function
khelif96 May 6, 2021
c92c6be
Begin mainline commits query
khelif96 May 6, 2021
2ff813c
Merge remote-tracking branch 'upstream/main' into EVG-13846
khelif96 May 6, 2021
1f70772
Add reminder
khelif96 May 6, 2021
1e529ea
Move build variant resolver logic to util file
khelif96 May 6, 2021
cd1092c
Make variant search take in array
khelif96 May 6, 2021
aabba9c
Filter by multiple tasks
khelif96 May 6, 2021
af013eb
support seraching build variant with filters
khelif96 May 6, 2021
0d31ad5
Support filtering by variant and tasks
khelif96 May 7, 2021
0f8b9ab
Add support for resolving statuses
khelif96 May 7, 2021
534ea5b
Rename to more generic type
khelif96 May 7, 2021
6352a33
Remove extra append
khelif96 May 7, 2021
150573b
Add tests
khelif96 May 7, 2021
d1b6d15
typo
khelif96 May 7, 2021
16afea1
Merge branch 'main' into EVG-13846
khelif96 May 7, 2021
24ad77d
Create universal build variant resolver
khelif96 May 10, 2021
066f6bc
Remove unused fields
khelif96 May 10, 2021
7eb1394
Remove unused options
khelif96 May 10, 2021
7bcc8b7
Update tests
khelif96 May 10, 2021
05fcbe7
make generateBuildVariant private
khelif96 May 10, 2021
37e0956
CR
khelif96 May 10, 2021
caa8948
Refactor fields
khelif96 May 10, 2021
347dbfa
Merge branch 'main' into EVG-13846
khelif96 May 11, 2021
836742a
Merge remote-tracking branch 'upstream/main' into EVG-13846
khelif96 May 12, 2021
b2acd6a
Consolidate types for versions and add pagination
khelif96 May 12, 2021
8fd5195
Roll up unactivated versions
khelif96 May 12, 2021
fcabe67
Remove limit logic
khelif96 May 12, 2021
d021e83
Add filter criteria that was removed
khelif96 May 12, 2021
78c2428
Update tests
khelif96 May 12, 2021
cf41c70
prettify
khelif96 May 12, 2021
49381c7
Add pagination tests
khelif96 May 12, 2021
eb04ee1
Fix nil conditional
khelif96 May 14, 2021
cf1249e
Add tests
khelif96 May 17, 2021
ffdb1d4
CR cleanup
khelif96 May 17, 2021
a0656b6
Merge branch 'main' into EVG-13846
khelif96 May 17, 2021
c52b11c
omit unused fields
khelif96 May 17, 2021
ae2d5ea
omit unused fields
khelif96 May 17, 2021
8170230
Pass value in directly
khelif96 May 17, 2021
881222f
remove [:]
khelif96 May 17, 2021
448334e
Apply default limit value
khelif96 May 17, 2021
4a7303c
Cleanup logic and add comments
khelif96 May 17, 2021
17501e2
Update tests to include rolled up case
khelif96 May 17, 2021
d258563
Merge remote-tracking branch 'upstream/main' into EVG-13846
khelif96 May 17, 2021
6c75deb
Default nil case
khelif96 May 17, 2021
77aa154
Should resolve build variants for activated == nil
khelif96 May 17, 2021
0da46d6
CR
khelif96 May 18, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions graphql/generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

111 changes: 43 additions & 68 deletions graphql/resolvers.go
Original file line number Diff line number Diff line change
Expand Up @@ -825,14 +825,7 @@ func (r *patchResolver) TaskStatuses(ctx context.Context, obj *restModel.APIPatc
{Key: task.DisplayNameKey, Order: 1},
}
opts := data.TaskFilterOptions{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you don't need to specify any of the fields you don't want to use; the defaults are the same as what you're specifying.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what I meant here was that you should just say:
opts := data.TaskFilterOptions{Sorts: defaultSort} rather than specifying the fields that you don't need (same below).

Statuses: []string{},
BaseStatuses: []string{},
Variants: []string{},
TaskNames: []string{},
Page: 0,
Limit: 0,
FieldsToProject: []string{},
Sorts: defaultSort,
Sorts: defaultSort,
}
tasks, _, err := r.sc.FindTasksByVersion(*obj.Id, opts)
if err != nil {
Expand Down Expand Up @@ -955,13 +948,7 @@ func (r *queryResolver) Patch(ctx context.Context, id string) (*restModel.APIPat
failedAndAbortedStatuses := append(evergreen.TaskFailureStatuses, evergreen.TaskAborted)
opts := data.TaskFilterOptions{
Statuses: failedAndAbortedStatuses,
BaseStatuses: []string{},
Variants: []string{},
TaskNames: []string{},
Page: 0,
Limit: 0,
FieldsToProject: []string{task.DisplayStatusKey},
Sorts: []task.TasksSortOrder{},
}
tasks, _, err := r.sc.FindTasksByVersion(id, opts)
if err != nil {
Expand Down Expand Up @@ -1172,14 +1159,13 @@ func (r *queryResolver) PatchTasks(ctx context.Context, patchID string, sorts []
}
}
opts := data.TaskFilterOptions{
Statuses: statuses,
BaseStatuses: baseStatuses,
Variants: []string{variantParam},
TaskNames: []string{taskNameParam},
Page: pageParam,
Limit: limitParam,
FieldsToProject: []string{},
Sorts: taskSorts,
Statuses: statuses,
BaseStatuses: baseStatuses,
Variants: []string{variantParam},
TaskNames: []string{taskNameParam},
Page: pageParam,
Limit: limitParam,
Sorts: taskSorts,
}
tasks, count, err := r.sc.FindTasksByVersion(patchID, opts)
if err != nil {
Expand Down Expand Up @@ -2526,47 +2512,42 @@ func (r *queryResolver) BbGetCreatedTickets(ctx context.Context, taskID string)

// Will return an array of activated and unactivated versions
func (r *queryResolver) MainlineCommits(ctx context.Context, options MainlineCommitsOptions) (*MainlineCommits, error) {

opts := model.MainlineCommitVersionOptions{
Activated: true,
SkipOrderNumber: 0,
Limit: 0,
projectId, err := model.GetIdForProject(options.ProjectID)
if err != nil {
return nil, ResourceNotFound.Send(ctx, fmt.Sprintf("Could not find project with id: %s", options.ProjectID))
}
if options.Limit != nil {
opts.Limit = *options.Limit
limit := model.DefaultMainlineCommitVersionLimit
if utility.FromIntPtr(options.Limit) != 0 {
limit = utility.FromIntPtr(options.Limit)
}
if options.SkipOrderNumber != nil {
opts.SkipOrderNumber = *options.SkipOrderNumber
opts := model.MainlineCommitVersionOptions{
Activated: true,
Limit: limit,
SkipOrderNumber: utility.FromIntPtr(options.SkipOrderNumber),
}

activatedVersions, err := model.GetMainlineCommitVersionsWithOptions(options.ProjectID, opts)
activatedVersions, err := model.GetMainlineCommitVersionsWithOptions(projectId, opts)
if err != nil {
return nil, ResourceNotFound.Send(ctx, fmt.Sprintf("Error getting activated versions %s", err.Error()))
return nil, ResourceNotFound.Send(ctx, fmt.Sprintf("Error getting activated versions: %s", err.Error()))
}

opts = model.MainlineCommitVersionOptions{
Activated: false,
SkipOrderNumber: 0,
Limit: 0,
}
if options.Limit != nil {
opts.Limit = *options.Limit
}
if options.SkipOrderNumber != nil {
opts.SkipOrderNumber = *options.SkipOrderNumber
SkipOrderNumber: utility.FromIntPtr(options.SkipOrderNumber),
}

unactivatedVersions, err := model.GetMainlineCommitVersionsWithOptions(options.ProjectID, opts)
unactivatedVersions, err := model.GetMainlineCommitVersionsWithOptions(projectId, opts)
if err != nil {
return nil, ResourceNotFound.Send(ctx, fmt.Sprintf("Error getting unactivated versions %s", err.Error()))
return nil, ResourceNotFound.Send(ctx, fmt.Sprintf("Error getting unactivated versions: %s", err.Error()))
}

versions := append(activatedVersions, unactivatedVersions...)
sort.Slice(versions, func(i, j int) bool {
return versions[i].RevisionOrderNumber > versions[j].RevisionOrderNumber
})
var mainlineCommits MainlineCommits
activatedVersionCount := 0
for _, v := range versions {
if activatedVersionCount == *options.Limit {
if activatedVersionCount == limit {
break
}
apiVersion := restModel.APIVersion{}
Expand All @@ -2582,14 +2563,7 @@ func (r *queryResolver) MainlineCommits(ctx context.Context, options MainlineCom
{Key: task.DisplayNameKey, Order: 1},
}
opts := data.TaskFilterOptions{
Statuses: []string{},
BaseStatuses: []string{},
Variants: []string{},
TaskNames: []string{},
Page: 0,
Limit: 0,
FieldsToProject: []string{},
Sorts: defaultSort,
Sorts: defaultSort,
}
tasks, _, err := r.sc.FindTasksByVersion(v.Id, opts)
if err != nil {
Expand All @@ -2609,24 +2583,37 @@ func (r *queryResolver) MainlineCommits(ctx context.Context, options MainlineCom
}
mainlineCommitVersion := MainlineCommitVersion{}

// If a version is activated we append it directly to our returned list of mainlineCommits
// If a version is not activated we roll up all the unactivated versions that are sequentially near each other
// into a single MainlineCommitVersion and then append it to our returned list
if utility.FromBoolPtr(v.Activated) {
activatedVersionCount++
mainlineCommits.NextPageOrderNumber = &v.RevisionOrderNumber
mainlineCommitVersion.Version = &apiVersion
mainlineCommits.Versions = append(mainlineCommits.Versions, &mainlineCommitVersion)

} else {
// If we have any versions already we should check the most recent one first otherwise create a new one
if len(mainlineCommits.Versions) > 0 {
lastMainlineCommit := mainlineCommits.Versions[len(mainlineCommits.Versions)-1]

// If the previous mainlineCommit contains rolled up unactivated versions append the latest RolledUp unactivated version
if lastMainlineCommit.RolledUpVersions != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can check this in the go playground, but I believe appending to a nil list does work. Otherwise if there aren't any rolled up versions yet, you're just omitting the version completely at this step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I added some comments and moved the logic a little bit to make it more clear what this is doing. I also updated the graphql tests to capture this scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for these comments!

lastMainlineCommit.RolledUpVersions = append(lastMainlineCommit.RolledUpVersions, &apiVersion)
} else {
mainlineCommitVersion.RolledUpVersions = []*restModel.APIVersion{&apiVersion}
}

} else {
mainlineCommitVersion.RolledUpVersions = []*restModel.APIVersion{&apiVersion}
mainlineCommits.Versions = append(mainlineCommits.Versions, &mainlineCommitVersion)

}

}

// Only add a mainlineCommit if a new one was added and its not a modified existing RolledUpVersion
if mainlineCommitVersion.Version != nil || mainlineCommitVersion.RolledUpVersions != nil {
mainlineCommits.Versions = append(mainlineCommits.Versions, &mainlineCommitVersion)
}
}

return &mainlineCommits, nil
Expand All @@ -2639,19 +2626,7 @@ func (r *versionResolver) BuildVariants(ctx context.Context, v *restModel.APIVer
if !utility.FromBoolPtr(v.Activated) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to consider if v.Activated is nil, or would the MainlineCommits resolver have been guaranteed to hit first?

Copy link
Contributor Author

@khelif96 khelif96 May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it is guaranteed to have been hit first. But if we want to use this for another field that returns a version it is not guaranteed. There will be a super low likely hood of it being nil unless someone decides to view a really old version that was introduced before this field (v.Activated) was introduced

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added handling for that edge case just in case.

return nil, nil
}
variants := []string{}
tasks := []string{}
statuses := []string{}
if options.Variants != nil {
variants = options.Variants
}
if options.Tasks != nil {
tasks = options.Tasks
}
if options.Statuses != nil {
statuses = options.Statuses
}
return generateBuildVariants(ctx, r.sc, *v.Id, variants, tasks, statuses)
return generateBuildVariants(ctx, r.sc, *v.Id, options.Variants, options.Tasks, options.Statuses)
}

func (r *Resolver) Version() VersionResolver { return &versionResolver{r} }
Expand Down
35 changes: 35 additions & 0 deletions graphql/tests/mainlineCommits/data.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,26 @@
"author": "mohamed.khelif",
"status": "created",
"order": 3
},
{
"_id": "evergreen_version4",
"gitspec": "5e823e1f28baeaa22ae00823d83e03082cd148ab",
"identifier": "evergreen",
"r": "gitter_request",
"activated": false,
"author": "mohamed.khelif",
"status": "created",
"order": 4
},
{
"_id": "evergreen_version5",
"gitspec": "5e823e1f28baeaa22ae00823d83e03082cd148ab",
"identifier": "evergreen",
"r": "gitter_request",
"activated": true,
"author": "mohamed.khelif",
"status": "created",
"order": 5
}
],
"tasks" : [
Expand Down Expand Up @@ -70,6 +90,16 @@
"build_variant" : "enterprise-ubuntu1604-64",
"build_id": "evergreen_version2_build"

},
{
"_id": "task_5",
"version": "evergreen_version5",
"status": "success",
"display_name": "Some Other Task",
"activated": true,
"build_variant" : "enterprise-ubuntu1604-64",
"build_id": "evergreen_version5_build"

}
],
"project_ref": [
Expand All @@ -88,6 +118,11 @@
"_id": "evergreen_version2_build",
"build_variant": "enterprise-ubuntu1604-64",
"display_name": "Ubuntu 16.04"
},
{
"_id": "evergreen_version5_build",
"build_variant": "enterprise-ubuntu1604-64",
"display_name": "Ubuntu 16.04"
}
]
}
2 changes: 1 addition & 1 deletion graphql/tests/mainlineCommits/queries/pagination.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
}
}
page2: mainlineCommits(
options: { projectID: "evergreen", limit: 1, skipOrderNumber: 2 }
options: { projectID: "evergreen", limit: 1, skipOrderNumber: 5 }
) {
versions {
version {
Expand Down
Loading