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

[DOP-4033]: Replicate shared.mk steps within Autobuilder code #917

Merged
merged 56 commits into from
Oct 26, 2023

Conversation

branberry
Copy link
Contributor

@branberry branberry commented Sep 26, 2023

Ticket

DOP-4033

Notes

This story converts the contents of the shared.mk file into TypeScript code.

Testing

  1. Checkout the branch and run npm ci as there are some new dependencies
  2. Run docker buildx build --file Dockerfile.local . --build-arg NPM_BASE_64_AUTH=<NPM_BASE_64_AUTH> --build-arg NPM_EMAIL=<NPM_EMAIL> -t autobuilder/local:latest ; the email and auth values can be found in parameter store
  3. Update the default profile in ~/.aws/credentials with temporary credentials from AWS; the credentials will have a randomly named profile that you should rename to default when pasting into the file
  4. Run docker run -v ~/.aws:/home/docsworker/.aws -e ENHANCED=true -e MONGO_ATLAS_USERNAME="<ATLAS_USERNAME>" -e MONGO_ATLAS_PASSWORD="<ATLAS_PASSWORD>" -e MONGO_ATLAS_HOST="<ATLAS_HOST>" autobuilder/local:latest using the staging environment values

@branberry branberry self-assigned this Oct 12, 2023
@branberry branberry marked this pull request as ready for review October 12, 2023 15:37
try {
await writeFileAsync(
prodFileName,
`GATSBY_BASE_URL=docs.mongodb.com
Copy link
Contributor

@rayangler rayangler Oct 16, 2023

Choose a reason for hiding this comment

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

We define GATSBY_BASE_URL=${baseUrl} below. Do we need this here as well?

/**
* This function is equivalent to a double redirect
* e.g. echo "Hello!" >> hello.txt
* @param param0
Copy link
Contributor

Choose a reason for hiding this comment

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

(Nit) Do we want to keep this param0?

export const checkIfPatched = async (repoDir: string) => !existsAsync(path.join(repoDir, 'myPatch.patch'));
export const getRepoDir = (repoName: string) => path.join(process.cwd(), `repos/${repoName}`);

export const RSTSPEC_FLAG =
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we export this but don't use it. The next-gen-parse file has its own instance of this. Should we remove?

patchId,
commitBranch,
}: StageParams) {
let hostedAtUrl = `${url}/${mutPrefix}/docsworker/${commitBranch}/`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should docsworker be docsworker-xlarge to maintain existing staging url path? Or are we intentionally changing it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I changed this for the local testing, but you're right. I'll need to rename it to docsworker-xlarge for when we use regular builds with these functions

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would we want to assign docsworker to a constant since it is used in two places?

const commandArgs = ['public', bucket, '--stage'];

if (patchId && projectName === mutPrefix) {
const [commitHash, patchId] = await Promise.all([getCommitHash(repoDir), getPatchId(repoDir)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why do we get the commit hash and patch ID again? Is there a reason we don't pass in and use patchId and commitHash from the call to prepareBuildAndGetDependencies earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

next-gen-stage checks these values for local jobs it looks like. Just making sure to copy the logic as closely as possible to what exists now.

And you're right, I should be using the commitHash and patchId from the prepare function

GATSBY_MANIFEST_PATH=${repoDir}/bundle.zip
GATSBY_PARSER_USER=${process.env.USER}
GATSBY_BASE_URL=${baseUrl}
GATSBY_PATH_PREFIX=${prefix}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

The frontend currently uses PATH_PREFIX instead of GATSBY_PATH_PREFIX

console.error('Options provided: ', options);

if (outputText) {
console.error(outputText.join());
Copy link
Collaborator

Choose a reason for hiding this comment

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

@branberry just out of curiosity, is there a reason why if there is outputText we are passing that to a console.error? Is there a case where this would provide an error message? I thought errorText was for capturing error messages.

Copy link
Contributor Author

@branberry branberry Oct 16, 2023

Choose a reason for hiding this comment

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

This is because a command can error out, but it still might have outputted to stdout. This is so we are able to see everything that was logged by this command when an error occurs. For example, if a command fails halfway through, there will be non-error logs that could be helpful to see, so I output it in case it does exist. I probably should check for outputText.length now that I think about it though.

I put it in console.error since this command errored out, but I am not opposed to logging it out like normal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation. I agree that it would probably make more sense to check the length because an empty array will still be truthy. Would it also make sense to maybe use console.log or console.info since console.error might be misleading because that output wasn't an error rather it was more information about at what point the command had an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! I think that makes sense. I'll change this to be a console.log instead.

console.error('Options provided: ', cmdToParams.options);

if (outputText) {
console.error('output', outputText.join());
Copy link
Collaborator

Choose a reason for hiding this comment

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

@branberry same here as well? Is there a reason why we aren't using a different console , i.e. console.log or console.info.

@caesarbell
Copy link
Collaborator

I may have missed this, but would we want to keep the benchmarking in the same place, or would we want to have it in the localApp.ts file or something similar if we have something like that for production use?

@caesarbell
Copy link
Collaborator

caesarbell commented Oct 16, 2023

Would we want to add the instructions (steps) that are in the description to live in the README somewhere?

createEnvProdFile(repoDir, projectName, baseUrl),
];

const dependencies = await Promise.all(commandPromises);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@branberry would we want to provide a way of catching or handling the error message if one of these promises are rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question, that is something I have been thinking about. I was thinking that maybe the error handling could occur in the place where this function gets called to simplify this code, but I am not set in stone on that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@branberry this was a pretty good read, on how and where to raise an error to be handled.

@@ -0,0 +1,46 @@
import { executeCliCommand, getCommitHash, getPatchId } from '../helpers';

interface StageParams {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would we want to just extend off of NextGenParseParams interface, since those types are shared with StageParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an option for sure, but I think it might be a bit more readable to list them all out. I also was thinking these interfaces to be isolated specifically for each function so they wouldn't need to reference another interface for another command.

That's me being nitpicky though, haha! What are your thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that is fair. and well thought out.

@@ -0,0 +1,46 @@
import { executeCliCommand, getCommitHash, getPatchId } from '../helpers';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@branberry do we want to remove getCommitHash and getPatchId as they seem to be not be used in this file anymore.

Copy link
Contributor

@rayangler rayangler left a comment

Choose a reason for hiding this comment

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

Lgtm!

@branberry branberry merged commit c3db19b into master Oct 26, 2023
6 of 7 checks passed
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