DOP-4209 #958

mayaraman19 · 2024-01-03T15:47:36Z

docs-monorepo/docs-mongodb-internal was not building without makefiles for multiple reasons:

retrieving the build dependencies from repos_branches did not take into consideration monorepo naming/directory structure, causing builds to just be pulling from docs-monorepo/cloud-docs since that was the first monorepo entry to show up in repos_branches.
migrating from makefiles did not take into account which error codes should/shouldn't cause the build to fail.

These have since been fixed, however, and docs-monorepo projects should be able to build now! (shoutout to @branberry for the help!)

mmeigs

Okay, some of these comments are a little long, so please reach out if you have any questions!

mmeigs · 2024-01-12T18:54:54Z

src/commands/src/helpers/dependency-helpers.ts

  repoName: string,
  projectName: string,
  baseUrl: string,
-  buildDependencies: BuildDependencies,
  directory?: string


If we're not getting dependencies in this function anymore, can we rename this function? prepareBuild could be fine by itself!

mmeigs · 2024-01-12T18:56:18Z

src/job/jobHandler.ts

  @throwIfJobInterupted()
  private async getBuildDependencies() {
-    const buildDependencies = await this._repoBranchesRepo.getBuildDependencies(this.currJob.payload.repoName);
+    const repoName = this.currJob.payload.repoName;
+    const directory = this.currJob.payload.repoName === MONOREPO_NAME ? this.currJob.payload.directory : undefined;
+    const buildDependencies = await this._repoBranchesRepo.getBuildDependencies(repoName, directory);
    if (!buildDependencies) return [];
+    await this._logger.save(this._currJob._id, 'Identified Build dependencies');
    return buildDependencies;
  }

  @throwIfJobInterupted()
-  private async getAndBuildDependencies() {
+  private async getAndDownloadBuildDependencies() {
+    const repoName = this.currJob.payload.repoName;
+    const directory = this.currJob.payload.repoName === MONOREPO_NAME ? this.currJob.payload.directory : undefined;
    const buildDependencies = await this.getBuildDependencies();
-    const commands = await downloadBuildDependencies(buildDependencies, this.currJob.payload.repoName);
-    this._logger.save(this._currJob._id, commands.join('\n'));
+    const commands = await downloadBuildDependencies(buildDependencies, this.currJob.payload.repoName, directory);
+    await this._logger.save(this._currJob._id, `${commands.join('\n')}`);
  }


We can combine these again. The reason to separate is gone now, and also it would combine the redundancy of accessing the repoName and directory twice in quick succession.

mmeigs · 2024-01-12T18:57:34Z

README.md


+<<<<<<< HEAD
+`npm run debug -- -o 10gen -n 'cloud-docs'`
+=======
 `npm run debug -- -o 10gen -n cloud-docs`



nit: Make sure the merge finishes and gets rid of these characters

Regarding the diff: The quotes are not necessary, but they are also not an issue. Either way is fine, but I would lean toward all the examples showing the same way for each input

Just noting here that this entire file has multiple unfinished merge flags to take care of!

mmeigs · 2024-01-12T18:59:51Z

cdk-infra/lib/constructs/api/webhook-env-construct.ts

@@ -19,7 +19,7 @@ export class WebhookEnvConstruct extends Construct {
    const ssmPrefix = getSsmPathPrefix();
    const env = getEnv();

-    const featureFlagMonorepoPath = StringParameter.valueFromLookup(this, `${ssmPrefix}/flag/monorepo_path`);
+    const featureFlagMonorepoPath = StringParameter.valueFromLookup(this, `${ssmPrefix}/DOP-4127/flag/monorepo_path`);


Just noting that this needs to be changed back before merge

mmeigs · 2024-01-12T19:00:00Z

cdk-infra/lib/constructs/worker/worker-env-construct.ts

@@ -50,7 +50,7 @@ export class WorkerEnvConstruct extends Construct {
    // doing this for the time being, but I think we don't need to necessarily retrieve this from ssm for feature branches, nor would we want to in that case
    const previewBuildEnabled = StringParameter.valueFromLookup(this, `${ssmPrefix}/flag/preview_build/enabled`);
    const featureFlagUpdatePages = StringParameter.valueFromLookup(this, `${ssmPrefix}/flag/update_pages`);
-    const featureFlagMonorepoPath = StringParameter.valueFromLookup(this, `${ssmPrefix}/flag/monorepo_path`);
+    const featureFlagMonorepoPath = StringParameter.valueFromLookup(this, `${ssmPrefix}/DOP-4127/flag/monorepo_path`);


Note to change back before merge

mmeigs · 2024-01-12T19:04:05Z

src/commands/src/helpers/dependency-helpers.ts

  const commands: string[] = [];
  await Promise.all(
    buildDependencies.map(async (dependencyInfo) => {
-      const repoDir = getRepoDir(repoName);
+      const repoDir = directory ? getRepoDir(repoName, directory) : getRepoDir(repoName);


No need to do this conditional! The directory argument can be undefined. You can simple state:

const repoDir = getRepoDir(repoName, directory);

mmeigs · 2024-01-12T19:19:16Z

src/commands/src/helpers/dependency-helpers.ts

      await Promise.all(
-        dependencyInfo.dependencies.map((dep) => {
+        dependencyInfo.dependencies.map(async (dep) => {
          try {
            executeCliCommand({
              command: 'curl',
-              args: ['-SfL', dep.url, '-o', `${targetDir}/${dep.filename}`],
+              args: ['--max-time', '10', '-SfL', dep.url, '-o', `${targetDir}/${dep.filename}`],
+              options: options,
            });
          } catch (error) {
            console.error(
              `ERROR! Could not curl ${dep.url} into ${targetDir}/${dep.filename}. Dependency information: `,
              dependencyInfo
            );
+            return commands;
          }
          commands.push(`curl -SfL ${dep.url} -o ${targetDir}/${dep.filename}`);
        })


Hmm, interesting thing here. I could be wrong, but I believe that because we're returning a static value (even though it's a Promise because of the async keyword) it can return before the executeCliCommand function finishes.

I think this can be slightly changed to do what you're trying to do and cleaner too. If you take away the async keyword and add return keyword to the executeCliCommand invocation that will ensure that the Promise.all waits for each Promise to finish. Then move the commands.push line up into the try block before the return. Then you can completely delete the unused line return commands. Doing this will ensure that the Node thread will not continue on until all of these commands have either finished or errored.

I have an example below.

Like this:

await Promise.all( dependencyInfo.dependencies.map((dep) => { try { commands.push(`curl -SfL ${dep.url} -o ${targetDir}/${dep.filename}`); return executeCliCommand({ command: 'curl', args: ['--max-time', '10', '-SfL', dep.url, '-o', `${targetDir}/${dep.filename}`], options: options, }); } catch (error) { console.error( `ERROR! Could not curl ${dep.url} into ${targetDir}/${dep.filename}. Dependency information: `, dependencyInfo ); } }) )

These are all great points, @mmeigs. The one small nit with the example though is that we won't hit this catch block, I think. This is because we return the promise, but we don't await it, not giving the promise the opportunity to settle before the catch block can be entered.

I think we can add the async back and do return await executeCliCommand... so that it will wait for the promise to settle before returning.

This also has me thinking as well, do we want to do an allSettled here maybe as well? That way, we can know all of the dependencies that are failing for whatever reason.

Testing this out with some incorrect curl links but this approach still doesn't seem to catch the error 😞

Hmm, I wonder if doing return await executeCliCommand(...) we try:

const cliResult = await executeCliCommand(...); return cliResult;

in the try/catch? I wonder if return is sort of overwriting the await in a sense

this didn't work either 💔 i dropped a new ticket for this issue though!

branberry · 2024-01-16T15:43:06Z

We can ignore the failing Update Feature Branch Infrastructure jobs. This stems from an edge case with the new updated feature branch deploys that depend on being run as a part of the first job.

…into DOP-4209

mayaraman19 · 2024-01-16T16:34:20Z

src/commands/src/helpers/dependency-helpers.ts

@@ -63,36 +72,29 @@ export async function downloadBuildDependencies(buildDependencies: BuildDependen
      }
      commands.push(`mkdir -p ${targetDir}`);
      await Promise.all(


Note: I did not use allSettled here because it seemed to not exist with this version of Javascript?

What version of node are you using?

mayaraman19 · 2024-01-16T20:30:22Z

I think I'm going to bump error logging in downloadBuildDependencies to another ticket because I can't figure it out 😭

mmeigs

Thanks for getting this working!

branberry

LGTM!

mmeigs added 30 commits November 14, 2023 15:50

experiments, added nextGenParse call to jobHandler

f4b5188

stage ecs - no makefiles

8f941ba

alter snooty version

706af67

next gen html

1a63501

command key

0be0b24

parse

6c43e45

catch

e733122

logger

2442b6f

remove error

901a229

more logging

9aa2fd2

cwd

8bd6ade

repoDir correct

c11687b

quotations

68eed83

successful parse? odd address

e9d9b36

oas page build

dc4c2a1

xlarge

1873ce9

persistence module

478ebf3

quote

ab4f652

no parse

71751a4

try catch parse

38c0f7d

type error

e313e5f

no parse, use persistence and build

b97835c

log errors

97cbc8b

localApp working

2832b0e

remove /dist

1d45d72

add build deps & nextgenhtml

11bac2c

add logging to build deps

7aee7af

log html

afadedc

remove build deps for envs

e320a67

env vars

178e680

[DOP-4269]: Add text to error

97ae5f4

mayaraman19 marked this pull request as ready for review January 12, 2024 17:42

mayaraman19 requested review from mmeigs and branberry January 12, 2024 18:04

mmeigs reviewed Jan 12, 2024

View reviewed changes

mayaraman19 added 3 commits January 16, 2024 10:30

consolidating

068c8ee

Merge branch 'master' into DOP-4209

6af15c4

fixing merge stuff in readme

afd3064

mayaraman19 added 3 commits January 16, 2024 11:13

Merge branch 'DOP-4209' of https://github.com/mongodb/docs-worker-pool …

b7e6bab

…into DOP-4209

merge

802340e

async in dep helper

882dff2

mayaraman19 commented Jan 16, 2024

View reviewed changes

mayaraman19 added 7 commits January 16, 2024 11:44

error message

4ef5cf8

nvm removing error msg

6c05afa

trying

f06608f

seems like the best way for await idk

ce59317

one more attempt

a0d9a4b

Nvm

da37185

not working

20c2bc6

mayaraman19 requested a review from mmeigs January 16, 2024 20:31

branberry and others added 3 commits January 19, 2024 10:07

Merge branch 'master' into DOP-4209

57c3fda

trying this out

577fba4

nvm

fc56fc7

mmeigs approved these changes Jan 22, 2024

View reviewed changes

branberry approved these changes Jan 22, 2024

View reviewed changes

Merge branch 'master' into DOP-4209

bb41a5f

mayaraman19 merged commit 834e691 into master Jan 22, 2024
8 of 10 checks passed

mayaraman19 deleted the DOP-4209 branch January 22, 2024 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOP-4209 #958

DOP-4209 #958

mayaraman19 commented Jan 3, 2024 •

edited

Loading

mmeigs left a comment

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

mmeigs Jan 12, 2024

branberry Jan 16, 2024

mayaraman19 Jan 16, 2024

branberry Jan 19, 2024

mayaraman19 Jan 22, 2024

branberry commented Jan 16, 2024

mayaraman19 Jan 16, 2024

branberry Jan 17, 2024

mayaraman19 Jan 19, 2024

mayaraman19 commented Jan 16, 2024

mmeigs left a comment

branberry left a comment

DOP-4209 #958

DOP-4209 #958

Conversation

mayaraman19 commented Jan 3, 2024 • edited Loading

mmeigs 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

branberry commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayaraman19 commented Jan 16, 2024

mmeigs left a comment

Choose a reason for hiding this comment

branberry left a comment

Choose a reason for hiding this comment

mayaraman19 commented Jan 3, 2024 •

edited

Loading