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.

Sign up for GitHub

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

Jump to bottom

fix(frontend/docker, ci): fix borked Docker build due to Lerna v8 uplift #29725

Merged
merged 2 commits into from
Jul 30, 2024
Merged

fix(frontend/docker, ci): fix borked Docker build due to Lerna v8 uplift #29725

merged 2 commits into from
Jul 30, 2024

Conversation

hainenber
Copy link
Contributor

@hainenber hainenber commented Jul 27, 2024 •
edited
Loading

fix(frontend/docker, ci): fix borked Docker build due to Lerna v8 uplift

SUMMARY

Closes #29658

Remove --no-optional in docker-frontend.sh to allow installing @nx optional dependencies which is necessary for Lerna v8

Also upgrading prop-types to next minor version to assess reproducibility of issue raised in PR #29723

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@hainenber
fix(docker): install optional @nx dep for Lerna v8+
c5c53bf 
Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber
chore(build): uplift prop-types to 15.8.1
75d4874 
Signed-off-by: hainenber <dotronghai96@gmail.com>
@dosubot dosubot bot added the infra:container Infra container and K8s label Jul 27, 2024
hainenber
hainenber commented Jul 27, 2024
View reviewed changes
docker/docker-frontend.sh
@@ -26,7 +26,7 @@ fi
if [ "$BUILD_SUPERSET_FRONTEND_IN_DOCKER" = "true" ]; then
cd /app/superset-frontend
npm install -f --no-optional --global webpack webpack-cli
npm install -f --no-optional
npm install -f
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mistercrunch I saw this flag was added previously but unsure the context. I believe there's a historical reason for it? 👀

Copy link
Member

Choose a reason for hiding this comment

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

I did some git archeology and it looks like --no-optional goes back to #10766 (2020) and there's not a clear explanation. I vote going vanilla and doing a very simple npm install instead of those 2 lines. Webpack packages should install as its just a devDependencies, and they'll install normally as long as we don't specify --production, which we don't want to do here.

Copy link
Member

Choose a reason for hiding this comment

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

tagging @eschutho as I believe she ran in an issue similar to the one you're fixing here.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... looks like @craig-rueda added that a looooong time ago. I'm not sure if there are any consequences to its removal ¯\_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

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

eek... duplicated effort. That's what I get for revisiting an open tab without refreshing.

Copy link
Member

Choose a reason for hiding this comment

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

@rusackas I really think it's ok to remove this --no-optional as it's non-standard and in this context would only apply BUILD_SUPERSET_FRONTEND_IN_DOCKER" = "true" which really should align with other build context (if you're running a dev env outside docker for instance).

Copy link
Member

Choose a reason for hiding this comment

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

This lgtm. I'll remove the pinned optional dependencies in my pr.

@hainenber hainenber changed the title fix(docker, ci): fix borked build due to Lerna v8 uplift fix(frontend/docker, ci): fix borked Docker build due to Lerna v8 uplift Jul 27, 2024
@hainenber
Copy link
Contributor Author

@eschutho I'm not able to reproduce your CI issue as this PR's frontend-build has installed FE dependencies with success 👀

https://github.com/apache/superset/actions/runs/10120546422/job/27990411018?pr=29725

@rusackas rusackas requested a review from eschutho July 29, 2024 18:04
@mistercrunch
Copy link
Member

My vote is to replace those two lines with a single, simple npm install

@rusackas rusackas merged commit 8891f04 into apache:master Jul 30, 2024
43 of 44 checks passed
@hainenber hainenber deleted the fix-borked-build-due-to-lerna-uplift branch July 30, 2024 18:28
@mistercrunch
Copy link
Member

The webpack install is not necessary, simplifying further here #29779

@sadpandajoe
Copy link
Member

@supersetbot label 4.1

@github-actions github-actions bot added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Jul 30, 2024
sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Jul 30, 2024
@hainenber @sadpandajoe
fix(frontend/docker, ci): fix borked Docker build due to Lerna v8 upl…
e5e293e 
…ift (apache#29725)

Signed-off-by: hainenber <dotronghai96@gmail.com>
(cherry picked from commit 8891f04)
@sadpandajoe
Copy link
Member

@supersetbot unlabel 4.1

@github-actions github-actions bot removed the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Aug 1, 2024
@sadpandajoe sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Nov 13, 2024
sadpandajoe pushed a commit that referenced this pull request Nov 13, 2024
@hainenber @sadpandajoe
fix(frontend/docker, ci): fix borked Docker build due to Lerna v8 upl…
72df46a 
…ift (#29725)

Signed-off-by: hainenber <dotronghai96@gmail.com>
(cherry picked from commit 8891f04)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rusackas rusackas rusackas approved these changes

@craig-rueda craig-rueda Awaiting requested review from craig-rueda

@villebro villebro Awaiting requested review from villebro

@mistercrunch mistercrunch Awaiting requested review from mistercrunch

@eschutho eschutho Awaiting requested review from eschutho

Assignees
No one assigned
Labels
dependencies:npm infra:container Infra container and K8s size/XS v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

superset_node build failure @nx/nx-linux-x64-gnu missing module
5 participants
@hainenber @mistercrunch @sadpandajoe @rusackas @eschutho