Skip to content

Conversation

msbt
Copy link
Contributor

@msbt msbt commented Oct 28, 2024

As the title suggests, this patch removes the deprecated node-sass package, updates node & npm, updates bootstrap, sass and sass-loader. As explained in #852 (comment) sass needed to be locked to a specific version in order to avoid deprecation warnings from bootstraps scss assets.

Summary of the changes / Why this is an improvement

Suggested by #852 (comment) this is a combination of both GH-852 and GH-863.

@cla-bot cla-bot bot added the cla-signed label Oct 28, 2024
@msbt msbt changed the title Remove node-sass, bump node to 22.10, bootstrap 5.3 and sass to 1.77.6 Remove node-sass, bump node to 22.10, bootstrap 5.3 and sass to 1.77.6 Oct 28, 2024
@msbt msbt requested a review from amotl October 28, 2024 12:18
@amotl
Copy link
Member

amotl commented Oct 28, 2024

Excellent to have all this bundled together into a single commit, derived from others, so we can easily revert, when needed. Thanks a stack!

What do you think about integrating this patch, despite the project officially being on a maintenance hiatus? At least, it would mitigate some flags on downstream security scanners, remember that admin ui is also bundled into OCI images.

@amotl amotl requested review from kneth and surister October 28, 2024 12:22
@msbt
Copy link
Contributor Author

msbt commented Oct 28, 2024

@amotl do we maybe need to update the test.yml to have also node 22? Seems to be stuck

@amotl
Copy link
Member

amotl commented Oct 28, 2024

CI/GHA seems to be stuck.

The current stalling in "Post Setup Node.js" is originating from saving its node_modules folder to the cache, I guess. I am sure it will run to completion.

Cache Size: ~29 MB

Don't know why that took so long. However, if it's not about "total size", it might easily also be about "number of files", that increases cache write duration. Otherwise, the lengthy runtime can also originate from datacenter/hardware congestion hiccups. The good thing is that on all other PRs, the cache will only be read, and not written.

Should CI also have node 22?

Yeah, please do like GH-863 is doing it. Thanks!

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Approving, but let's have another approval by at least one other person. Thanks!

/cc @surister, @kneth, @matriv, @seut

@msbt
Copy link
Contributor Author

msbt commented Oct 28, 2024

yes please, maybe I missed something on my local setup, more eyes = appreciated!

@amotl amotl merged commit 305e783 into main Oct 28, 2024
6 checks passed
@amotl amotl deleted the matthias/bootstrap-node-sass-update branch October 28, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants