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

khelif96 · 2021-05-07T20:02:13Z

This is the main query for the mainline commits page.

This supports most of the core features of the mainline commits flow.

Fetching both activated and unactivated versions
- Activated Versions are expanded and included extra metadata such as variants,
- Unactivated versions do not need to have nor include this extra metadata
Filtering by task status
Filter by task name/display name
Filtering by task status
Pagination
- This is a little difficult and i was hoping for some suggestions on how to achieve it.
- Ideally i want to handle the pagination on the db side of things using a $limit and $skip
- The complexity surrounds how we want to handle activated vs unactivated versions.
  - Activated versions are expanded and should count towards the limit of 7 activated versions on the screen at once.
  - Unactivated versions are collapsed and should not count towards the limit.
  - Ideally we could do a search with a limit of 7 active versions. and all the unactivated versions would not count towards this limit but still be returned in the result. And the skip function would hopefully work the same way. Skip the first n*7 activated results + all unactivated results that fall within.
  - If it's possible to do it in the db we could solve the issue of over fetching data. Alternatively we could do this logic in the resolver And intentionally over-fetch data and hope we have enough results to populate the page. But pagination logic will get pretty complicated, We wouldn't be able to simply pass in an arbitrary page number we would need some way to keep track of how many versions we skipped in the resolver somehow.

This notably does not include the ability to filter by test results. After speaking with @julianedwards it seems like it might be considerably more difficult to implement than expected so that is being held off for a later discussion.

ablack12 · 2021-05-10T19:27:29Z

rest/data/task.go

@@ -242,8 +242,8 @@ func (tc *DBTaskConnector) GetManifestByTask(taskId string) (*manifest.Manifest,

 // FindTasksByVersion gets all tasks for a specific version
 // Results can be filtered by task name, variant name and status in addition to being paginated and limited
-func (tc *DBTaskConnector) FindTasksByVersion(versionID string, statuses []string, baseStatuses []string, variant string, taskName string, page, limit int, fieldsToProject []string, sorts []task.TasksSortOrder) ([]task.Task, int, error) {
-	tasks, total, err := task.GetTasksByVersion(versionID, sorts, statuses, baseStatuses, variant, taskName, page, limit, fieldsToProject)
+func (tc *DBTaskConnector) FindTasksByVersion(versionID string, statuses []string, baseStatuses []string, variants []string, taskNames []string, page, limit int, fieldsToProject []string, sorts []task.TasksSortOrder) ([]task.Task, int, error) {


Sorry to ask this of you because it probably should've been done before, but this is getting really unwieldy; having this many inputs of the same type so close together introduces a lot of room for programmer error, and makes code very difficult to read. Would you mind refactoring this to take in a filterOptions struct or something, that contains the fields between statuses -> sort? (This also means you should just be able to pass in a blank filterOptions struct when you don't want to filter, which would be nice)

ablack12 · 2021-05-10T19:29:45Z

graphql/util.go

-				ets = append(ets, &at)
-			}
-			result.ExecutionTasksFull = ets
+func GenerateBuildVariants(ctx context.Context, sc data.Connector, versionId string, searchVariants []string, searchTasks []string, statuses []string) ([]*GroupedBuildVariant, error) {


I think this function can just be private (otherwise I'd probably ask that input is given with a struct rather than a collection of string maps, and a function header is provided)

ablack12 · 2021-05-10T19:31:12Z

graphql/util.go

+	}
+	tasks, _, err := sc.FindTasksByVersion(versionId, statuses, []string{}, searchVariants, searchTasks, 0, 0, []string{}, defaultSort)
+	if err != nil {
+		return nil, InternalServerError.Send(ctx, fmt.Sprintf("Error getting tasks for patch `%s`: %s", versionId, err))


Do we want this to be err.Error() explicitly?

ablack12 · 2021-05-10T19:31:32Z

graphql/util.go

+	if err != nil {
+		return nil, InternalServerError.Send(ctx, fmt.Sprintf("Error getting tasks for patch `%s`: %s", versionId, err))
+	}
+	for _, task := range tasks {


nit: avoid package names as variable names (in case we ever need to use the task collection in this function)

graphql/resolvers.go

ablack12 · 2021-05-10T19:43:28Z

graphql/models_gen.go

-	Tasks       []*model.APITask `json:"tasks"`
+type MainlineCommit struct {
+	Version     *model.APIVersion `json:"version"`
+	VersionMeta *VersionMeta      `json:"versionMeta"`


Can we call this versionMetadata instead? "meta" doesn't really mean anything and I'm not sure there's a reason to shorten it. Also, I'm wondering why you want a separate struct for metadata, as opposed to just filling out the necessary fields in the APIVersion. Is this to differentiate activated from not activated?

Yes this is so the buildVariant resolver isn't used on non activated versions. Since the types are different we're able to be explicit about what fields activated and non activated versions accept. I may try to build out a better type for this though.

I tried to implement something like this.

type MainlineCommit { version: Version } union Version = UnactivatedVersion | ActivatedVersion type UnactivatedVersion { id: String! createTime: Time! message: String! revision: String! author: String! status: String! order: Int! repo: String! project: String! branch: String! requester: String! activated: Boolean } type ActivatedVersion { startTime: Time! finishTime: Time! buildVariants(options: BuildVariantOptions): [GroupedBuildVariant] }

But a bug in gqlgen does not allow me to generate the types for union types and interfaces. So i think this is the best we can do for now.

ablack12 · 2021-05-10T19:44:16Z

model/version.go

+	if err := db.Aggregate(VersionCollection, pipeline, &res); err != nil {
+		return nil, errors.Wrapf(err, "error aggregating versions and builds")
+	}
+	if len(res) == 0 {


This if statement seems unnecessary

ablack12 · 2021-05-10T19:51:57Z

model/version.go

+	res := []Version{}
+
+	if err := db.Aggregate(VersionCollection, pipeline, &res); err != nil {
+		return nil, errors.Wrapf(err, "error aggregating versions and builds")


This doesn't get builds, right? Since it's only aggregating on the VersionCollection. Do you want it to also be getting builds?

model/version.go

ablack12 · 2021-05-13T14:52:13Z

graphql/resolvers.go

@@ -824,7 +824,17 @@ func (r *patchResolver) TaskStatuses(ctx context.Context, obj *restModel.APIPatc
 	defaultSort := []task.TasksSortOrder{
 		{Key: task.DisplayNameKey, Order: 1},
 	}
-	tasks, _, err := r.sc.FindTasksByVersion(*obj.Id, []string{}, []string{}, "", "", 0, 0, []string{task.DisplayStatusKey}, defaultSort)
+	opts := data.TaskFilterOptions{


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.

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).

model/version.go

ablack12 · 2021-05-13T15:21:28Z

model/task/task.go

 	var match bson.M = bson.M{}

 	// Allow searching by either variant name or variant display
-	if variant != "" {
+	if len(variants) > 0 {
+		variantsAsRegex := strings.Join(variants[:], "|")


What is this [:] for?

graphql/resolvers.go

ablack12 · 2021-05-13T16:24:03Z

graphql/resolvers.go

+	var mainlineCommits MainlineCommits
+	activatedVersionCount := 0
+	for _, v := range versions {
+		if activatedVersionCount == *options.Limit {


preference for FromIntPointer since it's safer
Do we set options.Limit to the default anywhere here or is this potentially just 0?

This hasn't been answered/updated ^

You are right i assumed gqlgen would have automatically applied a default for this field based off of us supplying a default value in the schema definition. But that doesn't appear to be the case.

graphql/resolvers.go

ablack12 · 2021-05-13T16:33:38Z

model/version.go

+	Activated       bool `json:"activated"`
+}
+
+func GetMainlineCommitVersionsWithOptions(projectName string, opts MainlineCommitVersionOptions) ([]Version, error) {


Can we write a unit test for this? I'd like to know if you have a Version inserted with Activated not set, if it gets included in the Activated: false match; otherwise if Activated is false we likely want to query both "false and unset"

ablack12 · 2021-05-17T13:56:56Z

graphql/resolvers.go

@@ -824,7 +824,17 @@ func (r *patchResolver) TaskStatuses(ctx context.Context, obj *restModel.APIPatc
 	defaultSort := []task.TasksSortOrder{
 		{Key: task.DisplayNameKey, Order: 1},
 	}
-	tasks, _, err := r.sc.FindTasksByVersion(*obj.Id, []string{}, []string{}, "", "", 0, 0, []string{task.DisplayStatusKey}, defaultSort)
+	opts := data.TaskFilterOptions{


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).

ablack12 · 2021-05-17T14:00:47Z

graphql/resolvers.go

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


should be versions: %s I think (same below)

ablack12 · 2021-05-17T14:03:27Z

graphql/resolvers.go

+	var mainlineCommits MainlineCommits
+	activatedVersionCount := 0
+	for _, v := range versions {
+		if activatedVersionCount == *options.Limit {


This hasn't been answered/updated ^

ablack12 · 2021-05-17T14:06:26Z

graphql/resolvers.go

+		} else {
+			if len(mainlineCommits.Versions) > 0 {
+				lastMainlineCommit := mainlineCommits.Versions[len(mainlineCommits.Versions)-1]
+				if lastMainlineCommit.RolledUpVersions != nil {


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.

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.

thanks for these comments!

ablack12 · 2021-05-17T14:07:49Z

graphql/util.go

+	defaultSort := []task.TasksSortOrder{
+		{Key: task.DisplayNameKey, Order: 1},
+	}
+	opts := data.TaskFilterOptions{


Same here: only need to specify Statuses, Variants, TaskNames, Sorts

ablack12 · 2021-05-17T14:08:05Z

graphql/resolvers.go

+	if options.Statuses != nil {
+		statuses = options.Statuses
+	}
+	return generateBuildVariants(ctx, r.sc, *v.Id, variants, tasks, statuses)


You can just pass in each option directly; there's no reason we can't pass in a nil list.

ablack12 · 2021-05-17T14:08:24Z

model/task/task.go

 	var match bson.M = bson.M{}

 	// Allow searching by either variant name or variant display
-	if variant != "" {
+	if len(variants) > 0 {
+		variantsAsRegex := strings.Join(variants[:], "|")


ablack12

Just a couple last thoughts from me!

ablack12 · 2021-05-17T19:36:09Z

graphql/resolvers.go

+		} else {
+			if len(mainlineCommits.Versions) > 0 {
+				lastMainlineCommit := mainlineCommits.Versions[len(mainlineCommits.Versions)-1]
+				if lastMainlineCommit.RolledUpVersions != nil {


thanks for these comments!

ablack12 · 2021-05-17T19:42:53Z

model/version.go

@@ -138,7 +139,7 @@ func (self *Version) SetActivated() error {
 }

 func (self *Version) SetNotActivated() error {
-	if utility.FromBoolPtr(self.Activated) {
+	if self.Activated != nil {


It's possible we'll want to use this function in the future to set something from activated to not activated (not sure why but it does seem like something that should work), so I think we only really want to return early if self.Activated is set explicitly to False already (you can use !utility.FromBoolTPtr to this, because the nil case defaults to true)

ablack12 · 2021-05-17T19:47:04Z

graphql/resolvers.go

+
+// Returns grouped build variants for a version. Will not return build variants for unactivated versions
+func (r *versionResolver) BuildVariants(ctx context.Context, v *restModel.APIVersion, options *BuildVariantOptions) ([]*GroupedBuildVariant, error) {
+	if !utility.FromBoolPtr(v.Activated) {


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

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

I added handling for that edge case just in case.

ablack12 · 2021-05-18T14:32:08Z

model/version.go

@@ -139,7 +139,7 @@ func (self *Version) SetActivated() error {
 }

 func (self *Version) SetNotActivated() error {
-	if self.Activated != nil {
+	if !utility.FromBoolPtr(self.Activated) {


So I suggested FromBoolTPtr here because right now, nil will default to false, which means that if self.Activated is false we'll exit early. FromBoolTPtr defaults nil to true, meaning we'll continue on to the update, which is what we want to happen (sorry about all the semantics, bool pointers are weird to reason about)

khelif96 added 17 commits April 28, 2021 12:42

Base mainline commits query

8184c81

Check if version is active and change return struct

d3ea4b6

Merge branch 'main' into EVG-13846

849a1ea

Fetch full version only when needed

e1bca53

Remove unused function

4a93c50

Begin mainline commits query

c92c6be

Merge remote-tracking branch 'upstream/main' into EVG-13846

2ff813c

Add reminder

1f70772

Move build variant resolver logic to util file

1e529ea

Make variant search take in array

cd1092c

Filter by multiple tasks

aabba9c

support seraching build variant with filters

af013eb

Support filtering by variant and tasks

0d31ad5

Add support for resolving statuses

0f8b9ab

Rename to more generic type

534ea5b

Remove extra append

6352a33

Add tests

150573b

khelif96 requested review from SupaJoon, malikchaya2, ablack12 and john-m-liu May 7, 2021 20:02

khelif96 mentioned this pull request May 7, 2021

EVG-13946 Update types for mainline commits pr evergreen-ci/spruce#748

Merged

khelif96 added 6 commits May 7, 2021 16:19

typo

d1b6d15

Merge branch 'main' into EVG-13846

16afea1

Create universal build variant resolver

24ad77d

Remove unused fields

066f6bc

Remove unused options

7eb1394

Update tests

7bcc8b7

ablack12 requested changes May 10, 2021

View reviewed changes

make generateBuildVariant private

05fcbe7

khelif96 requested a review from ablack12 May 12, 2021 21:23

ablack12 requested changes May 13, 2021

View reviewed changes

ablack12 reviewed May 13, 2021

View reviewed changes

khelif96 added 3 commits May 14, 2021 00:49

Fix nil conditional

eb04ee1

Add tests

cf1249e

CR cleanup

ffdb1d4

khelif96 requested a review from ablack12 May 17, 2021 13:30

ablack12 requested changes May 17, 2021

View reviewed changes

khelif96 added 8 commits May 17, 2021 10:17

Merge branch 'main' into EVG-13846

a0656b6

omit unused fields

c52b11c

omit unused fields

ae2d5ea

Pass value in directly

8170230

remove [:]

881222f

Apply default limit value

448334e

Cleanup logic and add comments

4a7303c

Update tests to include rolled up case

17501e2

khelif96 requested a review from ablack12 May 17, 2021 16:32

Merge remote-tracking branch 'upstream/main' into EVG-13846

d258563

ablack12 removed the request for review from john-m-liu May 17, 2021 19:33

ablack12 requested changes May 17, 2021

View reviewed changes

khelif96 added 2 commits May 17, 2021 15:53

Default nil case

6c75deb

Should resolve build variants for activated == nil

77aa154

khelif96 requested a review from ablack12 May 17, 2021 20:36

ablack12 requested changes May 18, 2021

View reviewed changes

CR

0da46d6

khelif96 requested a review from ablack12 May 18, 2021 19:19

ablack12 approved these changes May 18, 2021

View reviewed changes

SupaJoon approved these changes May 19, 2021

View reviewed changes

khelif96 merged commit 4071b3c into main May 25, 2021

khelif96 deleted the EVG-13846 branch May 25, 2021 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

khelif96 commented May 7, 2021 •

edited

Loading

ablack12 May 10, 2021

ablack12 May 10, 2021

ablack12 May 10, 2021

ablack12 May 10, 2021

ablack12 May 10, 2021

khelif96 May 10, 2021

khelif96 May 11, 2021

ablack12 May 10, 2021

ablack12 May 10, 2021

ablack12 May 13, 2021

ablack12 May 17, 2021

ablack12 May 13, 2021

ablack12 May 17, 2021

ablack12 May 13, 2021

ablack12 May 17, 2021

khelif96 May 17, 2021

ablack12 May 13, 2021

ablack12 May 17, 2021

ablack12 May 17, 2021

ablack12 May 17, 2021

ablack12 May 17, 2021

khelif96 May 17, 2021

ablack12 May 17, 2021

ablack12 May 17, 2021

ablack12 May 17, 2021

ablack12 May 17, 2021

ablack12 left a comment

ablack12 May 17, 2021

ablack12 May 17, 2021

ablack12 May 17, 2021

khelif96 May 17, 2021 •

edited

Loading

khelif96 May 17, 2021

ablack12 May 18, 2021

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

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

Conversation

khelif96 commented May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ablack12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khelif96 May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khelif96 commented May 7, 2021 •

edited

Loading

khelif96 May 17, 2021 •

edited

Loading