DOP-4020: repos_branches to docsets #909

mmeigs · 2023-09-18T18:40:28Z

Ticket

Staging

https://mongodbcom-cdn.website.staging.corp.mongodb.com/docs-qa/drivers/node/master/
https://mongodbcom-cdn.website.staging.corp.mongodb.com/docs-qa/atlas/master/

Notes

As part of the monorepo project, our queries to repos_branches needs to migrate to query the docsets collection instead.

The Autobuilder's RepoBranchesRepository class and Persistence module were what needed to be modified for this migration.

Other quality of life improvements were found and altered throughout this PR. A little far-reaching. Will add comments to try to explain to help.

Parameter store in AWS now holds values for the docsets collection.

A unique index was added to the docsets collections for the project field.

mmeigs · 2023-09-19T16:18:40Z

api/controllers/v1/slack.ts

-
-function isUserEntitled(entitlementsObject: any): boolean {
-  return (entitlementsObject?.repos?.length ?? 0) > 0;
-}
-
-function isRestrictedToDeploy(userId: string): boolean {
-  const { restrictedProdDeploy, entitledSlackUsers } = c.get<any>('prodDeploy');
-  return restrictedProdDeploy && !entitledSlackUsers.includes(userId);
-}
-
-function prepResponse(statusCode, contentType, body) {
-  return {
-    statusCode: statusCode,
-    headers: { 'Content-Type': contentType },
-    body: body,
-  };
-}
-
-async function buildEntitledBranchList(entitlement: any, repoBranchesRepository: RepoBranchesRepository) {
-  const entitledBranches: string[] = [];
-  for (const repo of entitlement.repos) {
-    const [repoOwner, repoName] = repo.split('/');
-    const branches = await repoBranchesRepository.getRepoBranches(repoName);
-    for (const branch of branches) {
-      let buildWithSnooty = true;
-      if ('buildsWithSnooty' in branch) {
-        buildWithSnooty = branch['buildsWithSnooty'];
-      }
-      if (buildWithSnooty) {
-        entitledBranches.push(`${repoOwner}/${repoName}/${branch['gitBranchName']}`);
-      }
-    }
-  }
-  return entitledBranches.sort();
-}
-
-function getQSString(qs: string) {
-  const key_val = {};
-  const arr = qs.split('&');
-  if (arr) {
-    arr.forEach((keyval) => {
-      const kvpair = keyval.split('=');
-      key_val[kvpair[0]] = kvpair[1];
-    });
-  }
-  return key_val;
-}


Realized these functions were identical and duplicated between v1 and v2. Rolled them out to a handlers file same as others we've done in the past month.

mmeigs · 2023-09-19T16:20:17Z

config/custom-environment-variables.json

@@ -14,7 +14,6 @@
  "githubBotPW": "GITHUB_BOT_PASSWORD",
  "fastlyDochubMap": "FASTLY_DOCHUB_MAP",
  "entitlementCollection": "USER_ENTITLEMENT_COL_NAME",
-  "reposBranchesCollection": "REPOS_BRANCHES_COL_NAME",


Unused almost-duplicate...? There were three of these. Removed here and below.
reposBranches vs repoBranches is how to spot the duplicate.

mmeigs · 2023-09-19T16:21:00Z

infrastructure/ecs-main/ecs_service.yml

@@ -47,7 +47,9 @@ Resources:
            - Name: JOB_QUEUE_COL_NAME
              Value: ${self:custom.jobCollection}
            - Name: REPO_BRANCHES_COL_NAME
-              value:  ${self:custom.repoBranchesCollection}
+              Value: ${self:custom.repoBranchesCollection}


Changed capitalizaton to match others.

mmeigs · 2023-09-19T16:21:43Z

serverless.yml

@@ -114,7 +115,8 @@ webhook-env-core: &webhook-env-core
  GITHUB_SECRET: ${self:custom.githubSecret}
  GITHUB_DELETION_SECRET: ${self:custom.githubDeletionSecret}
  GITHUB_BOT_PASSWORD: ${self:custom.githubBotPW}
-  REPO_BRANCHES_COL_NAME:  ${self:custom.repoBranchesCollection}
+  REPO_BRANCHES_COL_NAME: ${self:custom.repoBranchesCollection}


extra space

mmeigs · 2023-09-19T16:23:19Z

src/onDemandApp.ts

@@ -22,13 +23,13 @@ let githubCommandExecutor: GithubCommandExecutor;
 let jobRepository: JobRepository;
 let hybridJobLogger: HybridJobLogger;
 let repoEntitlementRepository: RepoEntitlementsRepository;
-let repoBranchesRepository: RepoBranchesRepository;


Almost-duplicate again. Unused. Real variable is below on line 31.

mmeigs · 2023-09-19T16:24:01Z

src/repositories/docsetsRepository.ts

+  /**
+   * Compares the project path from a monorepo push event, and compares it with
+   * what is configured in the docset entry in Atlas.
+   * @param path The project path where the snooty.toml file exists from the monorepo.
+   * This path will reflect the current project path from a given commit.
+   * @param projectName The project name for the docset entry.
+   * @returns A boolean representing whether or not the configured docset entry snooty_toml path
+   * matches the path found in GitHub.
+   */
+  async checkSnootyTomlPath(path: string, projectName: string) {
+    const query = { project: projectName };
+    try {
+      const docsetObject = await this.findOne(
+        query,
+        `Mongo Timeout Error: Timedout while retrieving repos entry for ${path}`
+      );
+
+      if (!docsetObject) {
+        console.warn(`WARNING: The docset does not exist for the following project: ${projectName} \n path: ${path}`);
+
+        return false;
+      }
+
+      return docsetObject.directories.snooty_toml === path;
+    } catch (error) {
+      console.warn(
+        `WARNING: Error occurred when retrieving project path for ${projectName}. The following path was provided: ${path}`,
+        error
+      );
+      return false;
+    }
+  }


From a previous PR that was in flow at the same time as this one. This merges the two classes together.

mmeigs · 2023-09-19T16:24:14Z

src/repositories/repoBranchesRepository.ts

-  async getConfiguredBranchesByGithubRepoName(repoName: string): Promise<any> {
-    const query = { repoName: repoName };
-    const reposObject = await this.findOne(
-      query,
-      `Mongo Timeout Error: Timedout while retrieving repos entry for ${repoName}`
-    );
-    if (reposObject?.branches) {
-      return {
-        branches: reposObject.branches,
-        repoName: reposObject.repoName,
-        status: 'success',
-      };
-    } else {
-      return { status: 'failure' };
-    }
-  }


Unused function. Deleted.

mmeigs · 2023-09-19T16:25:40Z

tests/unit/jobManager.test.ts

-      repoBranchesRepo
+      repoBranchesRepo,
+      docsetsRepo,
+      repoEntitlementsRepo


Added repoEntitlementsRepo because it was missing. Obviously not necessary because tests were not failing. But consistent.

rayangler

Mostly just questions from me as I try to understand things!

api/controllers/v1/github.ts

modules/persistence/src/services/metadata/repos_branches/index.ts

rayangler · 2023-09-20T15:10:25Z

modules/persistence/src/services/metadata/repos_branches/index.ts

    // if not already set, set cache value for repo_branches
    if (!internals[project]) {
-      internals[project] = res;
+      internals[project] = res[0];


Probably not relevant for the current state of our data, but I'm curious: since the matchCondition only expects a project and a branch, is it ever possible for 2 docs repos in the same docset with the same project and branch to both be in res? i.e. is the data we want always guaranteed to be at res[0]?

This is interesting. I could see that. I could always add checking for the repo that is "prodDeployable" however I believe there are instances where we actually want to use docs-mongodb-internal. So, that kind of throws a wrench in it.
This is the structure of the query that was findOne before, so I am now wondering if this needs to be a larger discussion about how we specify in these queries.

Gotcha, yeah, I think it might be a bit ambiguous since the previous queries didn't really take into account different repos with the same project either.

I'm okay with things as-is, or with the extra check for prodDeployable as long as we're getting back the intended repo info (since associated products seem to mostly work with prod deploy data). Seems like there's pretty minimal risk with keeping things like this for now anyways, so I don't think this should be a blocker

Agreed. And @seungpark does as well. I should have tagged this here.
For posterity, ticket created to address this.

tests/unit/repositories/docsetsRepository.test.ts

seungpark

@mmeigs what is the direction for the repo queries vs docsets queries. im assuming anything that is limited to usage of the properties you listed previously (project, bucket, url, prefix, directories) should be pointed to docset and existing repo branches queries should be fine? can we also reflect that in the A/C if this is the case

src/repositories/docsetsRepository.ts

seungpark · 2023-09-21T16:01:54Z

src/repositories/docsetsRepository.ts

+    ];
+  }
+
+  async getProjectByRepoName(repoName: string) {


would prefer return types in these functions. Promise

I'll work on that! These were straight from repoBranchesRepository.

seungpark · 2023-09-21T16:03:26Z

src/repositories/docsetsRepository.ts

+    const res = await cursor.toArray();
+    if (!res.length) {
+      const msg = `DocsetsRepository.getProjectByRepoName - Could not find project by repoName: ${repoName}`;
+      this._logger.info(this._repoName, msg);


should return or error here. or safeguard the res[0]?.project

You're totally right, thanks!

seungpark · 2023-09-21T16:07:22Z

src/repositories/docsetsRepository.ts

+  }
+
+  async getRepo(repoName: string): Promise<any> {
+    const aggregationPipeline = this.getAggregationPipeline('repoName', repoName);


as you mentioned in this comment, are we still going to query docsets for getting repo info?

getProjectByRepoName seems like it should be in docsets as project field should only be there in the future (CMIIW)

but getRepo and getRepoBranchesByRepoName seem like they can refer to the REPOS_BRANCHES collection

These queries are being used to access properties that are only available in the docsets collection as well as properties that are in the repos_branches collection! They need properties such as bucket and url

gotcha! so these are returning (essentially what is currently) a repo_branches entry, but it will be constructed from docsets since we are aiming to deprecate them from repos branches 👍

seungpark · 2023-09-21T16:11:09Z

src/repositories/docsetsRepository.ts

+        return res[0];
+      }
+    }
+    return { status: 'failure' };


do we want to throw here instead? i'm not sure if it'll ever be the case that cursor will be undefined (aggregate will throw)

Right again. I'll fix!

seungpark · 2023-09-21T16:14:00Z

api/controllers/v1/github.ts

  const jobPrefix = repoInfo?.prefix ? repoInfo['prefix'][env] : '';
  // TODO: Make job be of type Job
-  const job = await prepGithubPushPayload(body, repoBranchesRepository, jobPrefix);
+  const job = await prepGithubPushPayload(body, repoBranchesRepository, docsetsRepository, jobPrefix);


we are querying for getRepo twice (here and in getRepoBranchesByRepoName) can pass the repoInfo as a param

Nice, done!

src/repositories/docsetsRepository.ts

mmeigs · 2023-09-21T16:30:42Z

src/repositories/docsetsRepository.ts

+    ];
+  }
+
+  async getProjectByRepoName(repoName: string) {


I'll work on that! These were straight from repoBranchesRepository.

mmeigs · 2023-09-21T16:33:03Z

src/repositories/docsetsRepository.ts

+    const res = await cursor.toArray();
+    if (!res.length) {
+      const msg = `DocsetsRepository.getProjectByRepoName - Could not find project by repoName: ${repoName}`;
+      this._logger.info(this._repoName, msg);


You're totally right, thanks!

mmeigs · 2023-09-21T16:33:33Z

src/repositories/docsetsRepository.ts

+        return res[0];
+      }
+    }
+    return { status: 'failure' };


Right again. I'll fix!

rayangler

I think this lgtm

rayangler · 2023-09-21T18:19:57Z

modules/persistence/src/services/metadata/repos_branches/index.ts

    // if not already set, set cache value for repo_branches
    if (!internals[project]) {
-      internals[project] = res;
+      internals[project] = res[0];


Gotcha, yeah, I think it might be a bit ambiguous since the previous queries didn't really take into account different repos with the same project either.

I'm okay with things as-is, or with the extra check for prodDeployable as long as we're getting back the intended repo info (since associated products seem to mostly work with prod deploy data). Seems like there's pretty minimal risk with keeping things like this for now anyways, so I don't think this should be a blocker

seungpark

minor comment on v2 github controller but otherwise LGTM

seungpark · 2023-09-21T18:21:03Z

api/controllers/v2/github.ts

@@ -98,10 +101,10 @@ export const TriggerBuild = async (event: APIGatewayEvent): Promise<APIGatewayPr
  }

  const env = c.get<string>('env');
-  const repoInfo = await repoBranchesRepository.getRepo(body.repository.name);
+  const repoInfo = await docsetsRepository.getRepo(body.repository.name);


one more time here for double fetching getRepo

fixed! tysm

seungpark · 2023-09-21T18:25:29Z

src/repositories/docsetsRepository.ts

+  }
+
+  async getRepo(repoName: string): Promise<any> {
+    const aggregationPipeline = this.getAggregationPipeline('repoName', repoName);


gotcha! so these are returning (essentially what is currently) a repo_branches entry, but it will be constructed from docsets since we are aiming to deprecate them from repos branches 👍

seungpark · 2023-09-21T18:38:55Z

seems like test feature branch deploy is failing: Error: The stack named auto-builder-stack-enhancedApp-stg-DOP-4020-docsets-webhooks failed creation, it may need to be manually deleted from the AWS console: ROLLBACK_COMPLETE

mmeigs · 2023-09-21T18:56:24Z

seems like test feature branch deploy is failing: Error: The stack named auto-builder-stack-enhancedApp-stg-DOP-4020-docsets-webhooks failed creation, it may need to be manually deleted from the AWS console: ROLLBACK_COMPLETE

Asked @branberry about this and was told it's expected atm.
Posterity response: "feel free to ignore it. This is an issue with the initial deploy that I need to address. Basically, if there are errors when you first open the PR, the update feature branch job will fail since the queues will not have been created."

mmeigs added 18 commits September 14, 2023 09:49

find todos in main RepoBranchesRepository, consolidate slack helpers

3f27242

delete unused RepoBranchesRepository method

8d53507

replace getRepo everywhere with new DocsetsRepository getRepo

8216fd2

add docsetsRepository to each JobHandler and test

2dfc6bd

add docsetsRepo to JobHandlers

42b70f0

more test finds, missing references

eeed174

added env vars for docsetCollection

763f2c6

modify tests

776e30f

set to stage ecs

22b977c

add docsets to parameter store and serverless

5317f13

more env vars

f96335d

persistence module repos_branches migrate to docsets

3909513

work on tests in persistence module

5dbe09f

Merge branch 'master' into DOP-4020-docsets

ea80698

dont use preprd

4cf61c0

fixed persistence tests

8269fe0

merge two separate docsetsRepos

0cff1a1

deploy preprd

01200c2

mmeigs commented Sep 19, 2023

View reviewed changes

mmeigs requested review from branberry, seungpark and rayangler September 19, 2023 16:27

mmeigs marked this pull request as ready for review September 19, 2023 17:38

use .env

8352898

rayangler reviewed Sep 20, 2023

View reviewed changes

mmeigs added 3 commits September 20, 2023 13:35

debug failing test

e6f8bf4

all tests passing

b002f2e

Merge branch 'master' into DOP-4020-docsets

2b3a87a

seungpark reviewed Sep 21, 2023

View reviewed changes

mmeigs commented Sep 21, 2023

View reviewed changes

PR feedback

fc45dff

mmeigs requested review from rayangler and seungpark September 21, 2023 17:13

fix test

e2ca1d4

rayangler approved these changes Sep 21, 2023

View reviewed changes

seungpark approved these changes Sep 21, 2023

View reviewed changes

fix v2 to match v1, double query

b59e573

mmeigs added 2 commits September 22, 2023 10:32

Merge branch 'master' into DOP-4020-docsets

6d1e6d6

add warning to checkSnootyTomlPath

4727b68

mmeigs merged commit 01be34b into master Sep 22, 2023
6 of 7 checks passed

branberry mentioned this pull request Oct 13, 2023

Break up deploy for feature branch #927

Merged

DOP-4020: repos_branches to docsets #909

DOP-4020: repos_branches to docsets #909

Conversation

mmeigs commented Sep 18, 2023 • edited Loading

Ticket

Staging

Notes

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

rayangler 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

seungpark 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

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

rayangler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungpark 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

seungpark commented Sep 21, 2023

mmeigs commented Sep 21, 2023

mmeigs commented Sep 18, 2023 •

edited

Loading