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

[YUNIKORN-2481] Convert yunikorn-site to use pnpm #409

Closed
wants to merge 1 commit into from

Conversation

craigcondit
Copy link
Contributor

What is this PR for?

Use pnpm instead of yarn for yunikorn-site. This results in a much faster, more reliable build.

  • Update build scripts to use pnpm for dependency management
  • Ensure node_modules is not shared between Docker and host
  • Update Dockerfile to allow pre-caching pnpm dependencies

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2481

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@craigcondit craigcondit self-assigned this Mar 12, 2024
@craigcondit
Copy link
Contributor Author

craigcondit commented Mar 12, 2024

Most of the weirdness in the old build was due to node_modules being (incorrectly) shared between the host and the Docker environment. This has been fixed and the build steps re-ordered to allow caching intermediate Docker steps as long as dependencies have not changed. This should also eliminate the need to explicitly add the algolia search theme every time.

We also now check-in the lock file (like we do for other projects) so that we have consistent builds from one run to another.

- Update build scripts to use pnpm for dependency management
- Ensure node_modules is not shared between Docker and host
- Update Dockerfile to allow pre-caching pnpm dependencies
Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

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

Most of the changes looks good, tested in local and docker.

One more minor change required:

@@ -73,23 +76,13 @@ function run_base() {
if ! docker run -it --name yunikorn-site-local -d \
-p 3000:3000 \
-v "$PWD":/yunikorn-site \
--mount type=volume,dst=/yunikorn-site/node_modules \
Copy link
Contributor

Choose a reason for hiding this comment

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

It isolated the host's and docker's node_modules/ .
Could I confirm what is the previous weirdness in the old build? I wasn't aware of it before.

Most of the weirdness in the old build was due to node_modules being (incorrectly) shared between the host and the Docker environment.

Copy link
Contributor

@chenyulin0719 chenyulin0719 Mar 17, 2024

Choose a reason for hiding this comment

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

I think it would be better if we can explain the reason under run_base()'s comments.
ex:

function run_base() {
  # run docker container
  # mount the repo root to the container working dir,
  # so that changes made in the repo can trigger the web-server auto-update
  # Also prevent sharing node_modules/ between the host and the Docker environment to avoid potential conflict.

Copy link
Contributor Author

@craigcondit craigcondit Mar 17, 2024

Choose a reason for hiding this comment

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

Docker behavior is well documented already and I'd rather not clutter a simple function with a large comment 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.

It isolated the host's and docker's node_modules/ .

Could I confirm what is the previous weirdness in the old build? I wasn't aware of it before.

Most of the weirdness in the old build was due to node_modules being (incorrectly) shared between the host and the Docker environment.

We had all sorts of strange brokenness around algolia search having to be reinstalled constantly. I suspect there was platform-specific code being added and this caused issues when node_modules was shared between different platforms.

@craigcondit
Copy link
Contributor Author

craigcondit commented Mar 17, 2024

Most of the changes looks good, tested in local and docker.

One more minor change required:

I'd rather leave this in to prevent committing the lock file accidentally by other users since we previously used yarn. There will be many devs with old yarn lock files in their trees.

@craigcondit craigcondit dismissed chenyulin0719’s stale review March 17, 2024 11:28

@chenyulin0719 see my comments and please re-review.

Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

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

+1 LGTM

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