EVG-15206: Add CommitQueue to parser project #5085

hadjri · 2021-10-07T09:19:46Z

Description

Parser project struct has been updated to support commit queue configs in the yaml structure and all relevant places where properties are checked on the commit queue configs for a given project will now be checking a merged project ref struct with any necessary overriding properties from the parser project.

ablack12

This is coming up more and more; I'm leaning towards incorporating it into FindMergedProjectRef. (But I get if we want to do that at the end; should maybe make a ticket for it though)

ablack12 · 2021-10-07T15:58:32Z

model/project.go

@@ -66,7 +66,8 @@ type Project struct {
 	// The below fields can be set for the ProjectRef struct on the project page, or in the project config yaml.
 	// Values for the below fields set on this struct when TranslateProject is called for the project parser will
 	// take precedence over the project page and will be the configs used for a given project during runtime.
-	DeactivatePrevious bool `yaml:"deactivate_previous" bson:"deactivate_previous,omitempty"`
+	DeactivatePrevious bool               `yaml:"deactivate_previous" bson:"deactivate_previous,omitempty"`
+	CommitQueue        *CommitQueueParams `yaml:"commit_queue" bson:"commit_queue"`


The benefit of this being a pointer is that we can use omitempty to prevent saving an empty struct.

ablack12 · 2021-10-07T15:58:47Z

model/project_parser.go

@@ -89,7 +89,8 @@ type ParserProject struct {
 	// The below fields can be set for the ProjectRef struct on the project page, or in the project config yaml.
 	// Values for the below fields set on this struct will take precedence over the project page and will
 	// be the configs used for a given project during runtime.
-	DeactivatePrevious *bool `yaml:"deactivate_previous" bson:"deactivate_previous,omitempty"`
+	DeactivatePrevious *bool              `yaml:"deactivate_previous" bson:"deactivate_previous,omitempty"`
+	CommitQueue        *CommitQueueParams `yaml:"commit_queue" bson:"commit_queue"`


also omitempty

ablack12 · 2021-10-07T16:00:52Z

model/project_ref.go

@@ -2131,6 +2138,10 @@ func (p *ProjectRef) CommitQueueIsOn() error {
 	if p.IsPatchingDisabled() {
 		catcher.Add(errors.Errorf("patching is disabled for project '%s'", p.Id))
 	}
+	err := p.MergeWithParserProject("")
+	if err != nil {


can combine in one line if err := ... ; err != nil

ablack12 · 2021-10-07T16:00:58Z

model/project_ref.go

@@ -2131,6 +2138,10 @@ func (p *ProjectRef) CommitQueueIsOn() error {
 	if p.IsPatchingDisabled() {
 		catcher.Add(errors.Errorf("patching is disabled for project '%s'", p.Id))
 	}
+	err := p.MergeWithParserProject("")
+	if err != nil {
+		catcher.Add(errors.Wrap(err, "can't merge parser project with project ref"))


catcher.Wrapf

ablack12 · 2021-10-07T16:01:18Z

operations/commit_queue.go

@@ -419,6 +419,10 @@ func listCommitQueue(ctx context.Context, client client.Communicator, ac *legacy
 	if err != nil {
 		return errors.Wrapf(err, "can't find project for queue id '%s'", projectID)
 	}
+	err = projectRef.MergeWithParserProject("")
+	if err != nil {


combine with 423 (also below)

ablack12 · 2021-10-07T16:02:36Z

operations/commit_queue.go

@@ -690,7 +698,8 @@ func (p *moduleParams) addModule(ac *legacyClient, rc *legacyClient) error {

 func showCQMessageForProject(ac *legacyClient, projectID string) {
 	projectRef, _ := ac.GetProjectRef(projectID)
-	if projectRef != nil && projectRef.CommitQueue.Message != "" {
+	err := projectRef.MergeWithParserProject("")
+	if err != nil && projectRef != nil && projectRef.CommitQueue.Message != "" {


You can't combine these checks; MergeWithParserProject will (and rightly should) panic if projectRef is nil

ablack12 · 2021-10-07T16:04:33Z

rest/route/repo.go

@@ -277,7 +281,8 @@ func (h repoIDPatchHandler) validateBranchesForRepo(ctx context.Context, newRepo
 		githubCheckIds []string
 	}{}
 	for _, p := range mergedRepos {
-		if !p.IsEnabled() {
+		err = p.MergeWithParserProject("")


This isn't actually commit queue; are we allowing overall enabled to also be defined in yaml?

No, this was mistaken on my end for commit queue enabled check. Reverted.

ablack12 · 2021-10-07T16:05:33Z

rest/route/project.go

@@ -235,6 +235,10 @@ func (h *projectIDPatchHandler) Parse(ctx context.Context, r *http.Request) erro
 	if err != nil {
 		return errors.Wrap(err, "error finding original project")
 	}
+	err = oldProject.MergeWithParserProject("")


We don't want to do this; we use this to determine what the new project ref to be saved to the DB should be, so we'll end up saving project ref values.

ablack12 · 2021-10-07T16:06:40Z

service/project.go

@@ -150,6 +150,11 @@ func (uis *UIServer) projectPage(w http.ResponseWriter, r *http.Request) {
 	CQConflictingRefs := []string{}
 	githubChecksConflictingRefs := []string{}
 	for _, ref := range matchingRefs {
+		err = ref.MergeWithParserProject("")


We don't want to display yaml values on the UI like this

hadjri · 2021-10-08T09:11:37Z

This is coming up more and more; I'm leaning towards incorporating it into FindMergedProjectRef. (But I get if we want to do that at the end; should maybe make a ticket for it though)

I've thought about it more and I agree - I've created a small 2 point ticket to cover this refactoring to be done next sprint: https://jira.mongodb.org/browse/EVG-15562

ablack12 · 2021-10-08T17:36:52Z

operations/commit_queue.go

-	if err != nil && projectRef != nil && projectRef.CommitQueue.Message != "" {
-		grip.Info(projectRef.CommitQueue.Message)
+	if projectRef != nil && projectRef.CommitQueue.Message != "" {
+		if err := projectRef.MergeWithParserProject(""); err != nil {


Sorry for not catching earlier; you can't do this for the CLI; it needs to use rest routes to communicate with Evergreen, i.e. to get the version. GetProjectRef is an example of this. I would modify that function to send over the already merged project.

ablack12 · 2021-10-08T17:37:49Z

operations/commit_queue.go

-	err := projectRef.MergeWithParserProject("")
-	if err != nil && projectRef != nil && projectRef.CommitQueue.Message != "" {
-		grip.Info(projectRef.CommitQueue.Message)
+	if projectRef != nil && projectRef.CommitQueue.Message != "" {


You're moving this anyway but you'd want to look this up even if Message == ""

This reverts commit a9f9e0f.

hadjri added 4 commits October 7, 2021 03:55

EVG-15206: Add CommitQueue to parser project

3200650

EVG-15206: Update merge function and agent versions

f77e440

EVG-15206: Update merge function and agent versions

cbd8752

EVG-15206: Update merge function and agent versions

6b69bf1

hadjri marked this pull request as ready for review October 7, 2021 11:23

hadjri requested a review from ablack12 October 7, 2021 11:23

ablack12 reviewed Oct 7, 2021

View reviewed changes

EVG-15206: Refactor addressing review comments

87d719d

hadjri requested a review from ablack12 October 8, 2021 09:12

ablack12 requested changes Oct 8, 2021

View reviewed changes

EVG-15206: Move MergeWithParserProject inside fetchProjectRef

6150844

hadjri requested a review from ablack12 October 8, 2021 20:11

ablack12 approved these changes Oct 8, 2021

View reviewed changes

hadjri merged commit a9f9e0f into evergreen-ci:main Oct 13, 2021

ablack12 added a commit that referenced this pull request Oct 14, 2021

Revert "EVG-15206: Add CommitQueue to parser project (#5085)"

0db5f46

This reverts commit a9f9e0f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVG-15206: Add CommitQueue to parser project #5085

EVG-15206: Add CommitQueue to parser project #5085

hadjri commented Oct 7, 2021 •

edited

Loading

ablack12 left a comment

ablack12 Oct 7, 2021

ablack12 Oct 7, 2021

ablack12 Oct 7, 2021

ablack12 Oct 7, 2021

ablack12 Oct 7, 2021

ablack12 Oct 7, 2021

ablack12 Oct 7, 2021

hadjri Oct 8, 2021

ablack12 Oct 7, 2021

ablack12 Oct 7, 2021

hadjri commented Oct 8, 2021

ablack12 Oct 8, 2021

ablack12 Oct 8, 2021

EVG-15206: Add CommitQueue to parser project #5085

EVG-15206: Add CommitQueue to parser project #5085

Conversation

hadjri commented Oct 7, 2021 • edited Loading

Description

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

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

hadjri commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadjri commented Oct 7, 2021 •

edited

Loading