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

DEVPROD-7219 Remove db access from cli commands #7912

Merged
merged 16 commits into from
May 29, 2024

Conversation

bynn
Copy link
Contributor

@bynn bynn commented May 22, 2024

DEVPROD-7219

Description

Adding a db call to get project during API creation broke this function for the CLI
We now ignore the db calls when it comes from the CLI

When the function is called from the CLI, it returns the same value it used to return before the calls to the db were added

Testing

works locally

@bynn bynn requested a review from a team May 22, 2024 18:59
return errors.Errorf("project ref '%s' not found", projectIdentifier)
}
// Projects that use the GitHub merge queue cannot enqueue to the commit queue.
apiPatch.CanEnqueueToCommitQueue = (p.HasValidGitInfo() || p.IsGithubPRPatch()) && proj.CommitQueue.MergeQueue != model.MergeQueueGitHub
Copy link
Contributor

@ablack12 ablack12 May 22, 2024

Choose a reason for hiding this comment

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

This field is only used by the UI, right? Instead of including this here, could we instead remove CanEnqueueToCommitQueue from the APIPatch struct and instead just add a resolver for it? (like

func (r *patchResolver) CommitQueuePosition(ctx context.Context, obj *restModel.APIPatch) (*int, error) {
)

Copy link
Contributor

Choose a reason for hiding this comment

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

(@khelif96 is that reasonable?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this makes sense to me

@bynn bynn requested a review from ablack12 May 23, 2024 19:31
Copy link
Contributor

@ablack12 ablack12 left a comment

Choose a reason for hiding this comment

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

Discussed in DM moving this to a new patchResolver for the field rather than the main patch query resolver.

@bynn bynn requested review from khelif96 and ablack12 May 24, 2024 21:44
@bynn bynn merged commit cc3352b into evergreen-ci:main May 29, 2024
8 checks passed
@bynn bynn deleted the DEVPROD-7219 branch May 29, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants