Skip to content

Conversation

jotak
Copy link
Member

@jotak jotak commented Sep 26, 2025

Some build time optimization
On my machine, webpack build going from 138s down to 52s

Copy link

openshift-ci bot commented Sep 26, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kalmanmeth for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jotak jotak requested a review from jpinsonneau October 1, 2025 12:53
@jotak jotak added the needs-review Tells that the PR needs a review label Oct 1, 2025
@jotak
Copy link
Member Author

jotak commented Oct 1, 2025

/retest

@jpinsonneau
Copy link
Contributor

Should we rely on process.env.NODE_ENV === 'production' for such changes and have a build:production in package.json ?

@jotak
Copy link
Member Author

jotak commented Oct 8, 2025

@jpinsonneau why would it be only for prod? It seems good in both cases, no?

Copy link

codecov bot commented Oct 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.06%. Comparing base (418cd67) to head (dbbcc57).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1049      +/-   ##
==========================================
- Coverage   53.20%   53.06%   -0.15%     
==========================================
  Files         205      205              
  Lines       11115    11149      +34     
  Branches     1295     1300       +5     
==========================================
+ Hits         5914     5916       +2     
- Misses       4689     4717      +28     
- Partials      512      516       +4     
Flag Coverage Δ
uitests 55.91% <ø> (-0.07%) ⬇️
unittests 46.82% <ø> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jpinsonneau
Copy link
Contributor

We can use transpileOnly for production but we should also use fork-ts-checker-webpack-plugin to keep type checks:

https://webpack.js.org/guides/build-performance/#typescript-loader

Also, I'm not sure about the cache filesystem impact on those builds.

@jotak
Copy link
Member Author

jotak commented Oct 21, 2025

We can use transpileOnly for production but we should also use fork-ts-checker-webpack-plugin to keep type checks:

https://webpack.js.org/guides/build-performance/#typescript-loader

In fact, I think what I would really like is to decouple some of these steps (format check, linting, type check) from the build. It would not only speed up the development process, but also konflux builds for instance: if you think about it, a lot of time is wasted on duplicated work, especially as we're building for several architectures. I think we shouldn't care about linting and checks like that during downstream builds for instance. This could be something only checked on github CI per pull request, and after merged, it should be considered OK, no need to check it again in konflux

And the same would be possible for dev build, where we don't necessarily care about type-checking given that our IDE already care about that, and the build time is largely impacted by this step

@jpinsonneau
Copy link
Contributor

We can use transpileOnly for production but we should also use fork-ts-checker-webpack-plugin to keep type checks:
https://webpack.js.org/guides/build-performance/#typescript-loader

In fact, I think what I would really like is to decouple some of these steps (format check, linting, type check) from the build. It would not only speed up the development process, but also konflux builds for instance: if you think about it, a lot of time is wasted on duplicated work, especially as we're building for several architectures. I think we shouldn't care about linting and checks like that during downstream builds for instance. This could be something only checked on github CI per pull request, and after merged, it should be considered OK, no need to check it again in konflux

And the same would be possible for dev build, where we don't necessarily care about type-checking given that our IDE already care about that, and the build time is largely impacted by this step

Sure if we run the typechecks once in parallel I'm fine with that 👌

@jotak
Copy link
Member Author

jotak commented Oct 21, 2025

Fair enough, I'll rework this PR with that goal

@jotak
Copy link
Member Author

jotak commented Oct 21, 2025

@jpinsonneau done
tested with a typing issue intended: PR failure, as expected: https://github.com/netobserv/network-observability-console-plugin/actions/runs/18685639972/job/53277390843?pr=1049

@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Oct 21, 2025
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:08b738a

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=08b738a make set-plugin-image

Copy link
Contributor

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @jotak !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm needs-review Tells that the PR needs a review ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants