Skip to content

Conversation

@W-A-James
Copy link
Contributor

@W-A-James W-A-James commented May 4, 2023

Description

What is changing?

Is there new documentation needed for these changes?

No

What is the motivation for this change?

Ensuring we keep up with spec.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@W-A-James W-A-James changed the title test(NODE-4772): add new prose test test(NODE-4772): Test that mongocryptd is not spawned when shared library is loaded May 4, 2023
@W-A-James W-A-James marked this pull request as ready for review May 4, 2023 20:30
@baileympearson baileympearson self-assigned this May 8, 2023
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 8, 2023
@W-A-James W-A-James requested a review from baileympearson May 9, 2023 17:46
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to change this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was intentional. It didn't have execute permissions by default and whenever I test locally, I have to give it execute permissions, so I updated that here. I can add it to the "What is changing?" section of the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any other reason you can't just bash .evergreen/run-kms-servers.sh? I'm not opposed to adding execute permissions but we don't have them currently because in CI we intentionally run these scripts with bash because the default shell in evergreen is different on different hosts.

Screenshot 2023-05-09 at 3 09 15 PM

I was also confused by the changes because I thought Github was displaying the file size, not the permissions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No other reason than convenience.

On further inspection, if we're going to keep the permissions change, we should also add a shebang line to the top of that file so if it is invoked with ./.evergreen/run-kms-servers.sh it uses bash automatically.

….prose.test.js

Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
@W-A-James W-A-James requested a review from baileympearson May 9, 2023 20:49
@baileympearson baileympearson merged commit 41d4f0d into main May 11, 2023
@baileympearson baileympearson deleted the NODE-4772/test_mongocryptd_not_spawned_when_shlib_loaded branch May 11, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants