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

MI-85: Multi package managers & improvements #10

Merged
merged 11 commits into from
Nov 21, 2024

Conversation

kai-nguyen-aligent
Copy link
Contributor

This PR is a bit big because I also fix a couple of issues while using this pipeline:

  1. Support all 3 main package managers (npm, pnpm & yarn).
  2. Update documents as we now not only support Nx monorepo in this pipeline. We also support the [legacy polyrepo] (feat DO-1593: add support for non-mono repo #8)
  3. We used to rely on ts-node instead of trans-pile to Javascript.
  • I have removed ts-node from dependencies.
  • Added tsx to devDependencies for development.
  • Transpile the code to Javascript before building docker image.
  • We now run the pipeline directly using node. In the future, we also can pack this into a npm package and run it directly from Bitbucket pipeline via npx.
  1. Remove redundant pipe.yml. @tvhees I have no idea why this file exist in this repository. Do you recall why it's here & should it be removed or not?
  2. We used to only see plain white text even though Bitbucket pipeline support colours. Therefore, I added colour (and a small icon) into our pipe main console.log() & preserve colours from sub-commands. The result now look like so:
    image

@TheOrangePuff Will need your help on reviewing parts related to docker image building & workflow.

@kai-nguyen-aligent kai-nguyen-aligent requested review from tvhees, TheOrangePuff, a team and ryanrixxh and removed request for a team and TheOrangePuff October 9, 2024 01:22
Copy link

@ryanrixxh ryanrixxh left a comment

Choose a reason for hiding this comment

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

LGTM but need to run this by a DevOps

Copy link
Contributor

@TheOrangePuff TheOrangePuff left a comment

Choose a reason for hiding this comment

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

Couple comments

.github/workflows/build.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@kai-nguyen-aligent
Copy link
Contributor Author

@TheOrangePuff All issue has been addressed. I also update Dockerfile to match with recommended approach from https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#smaller-images-without-npmyarn.

@kai-nguyen-aligent
Copy link
Contributor Author

@TheOrangePuff Is there anything that block this PR to be merged? I understand that we do not have the final decision on which package manager we should use yet but this PR supports all 3 main one. Considering the following benefit, I think there is no harm of merging this even if we are going to stick with npm in the future:

  • Support colour in CI
  • More robust CI as ts-node has been removed. We run the code directly.

Copy link
Contributor

@TheOrangePuff TheOrangePuff left a comment

Choose a reason for hiding this comment

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

LGTM

@kai-nguyen-aligent kai-nguyen-aligent merged commit 69f156b into main Nov 21, 2024
1 check 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