Skip to content

Conversation

@wtrocki
Copy link
Member

@wtrocki wtrocki commented Feb 3, 2025

Proposed changes

CLOUDP-29823

With current bash based transformation it is hard to make any changes and effectively test them.
This PR introduces transformation engine in JS (as alternative to bash/regexp patterns).

Testing

I have tested input of both jobs and they produce exactly the same output.
Tests will be added in follow-up PR

Benefits

  • Easier to make follow up changes and maintain the code.
  • Unit tests possible
  • Runtime on my machine changed from 61 seconds to 2 seconds.
  • Removed couple immediate files/hacks

Follow up work/PRs

  • Add tests
  • Swap and Test CI/CD system to use JS targets.
  • Add new postman transformations to introduce service accounts and document PAK enablement

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works

Changes to Spectral

  • I have read the README file for Spectral Updates

Further comments

@wtrocki wtrocki requested a review from a team as a code owner February 3, 2025 17:12
Copy link
Collaborator

@lovisaberggren lovisaberggren left a comment

Choose a reason for hiding this comment

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

LGTM, one question on CJS vs ESM 👍


.PHONY: transform_collection_js
transform_collection_js:
node ./scripts/transform-postman.cjs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Any reason to use commonJS instead of ESM?

Copy link
Member Author

@wtrocki wtrocki Feb 6, 2025

Choose a reason for hiding this comment

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

ESM will require different setup (it did not work out of the box) so moved to cjs as it works out of the box with eslint. Will address this in follow up.

@wtrocki wtrocki merged commit c7dfbcf into main Feb 6, 2025
11 checks passed
@wtrocki wtrocki deleted the CLOUDP-298233 branch February 6, 2025 15:26
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.

2 participants