ambari icon indicating copy to clipboard operation
ambari copied to clipboard

AMBARI-25289 Upgrade Jquery and Bootstrap to latest versions

Open vanshuhassija opened this issue 2 years ago • 7 comments

What changes were proposed in this pull request?

The current version of jquery and bootstrap has vulnerabilities which are then resolved in the latest versions of the libraries. There is a major version upgrade from 1.9.1 to 3.7.1 for jquery and Bootstrap 3 to bootstrap 5 for bootstrap. A lot of breaking changes were introduced between the versions. With this PR, deprecated methods are removed and are replaced with the newly supported methods. The reference has been taken from the changelog of the dependencies.

How was this patch tested?

Made the changes and tested the intended behaviour of the app by running test cases and performing manual checks.

Please review Ambari Contributing Guide before opening a pull request.

vanshuhassija avatar Feb 01 '24 15:02 vanshuhassija

@vanshuhassija Great work. Due to many of Ambari's front-end libraries being outdated and no longer maintained, there's a need for gradual iteration and updating to newer versions. This PR is excellent. I'll first test this PR in my environment.

JiaLiangC avatar Apr 27 '24 02:04 JiaLiangC

@vanshuhassija After applying this patch, running into errors (OS: Win10 PowerShell, Node: v4.5.0, Yarn: v0.23.2), seems to encounter some minor issues. Below are the steps I followed:

  1. cd ambari-web
  2. yarn install --ignore-engines --pure-lockfile
  3. yarn build (use the script brunch build specified in package.json).
  4. Use Live Server to serve the public directory.

Encountered errors include:

Uncaught ReferenceError: Backbone is not defined

...
...

Uncaught SyntaxError: Identifier 'tooltipTriggerList' has already been declared

The second error is likely caused by the similarity in content between ambari-web/vendor/scripts/bootstrap-popover.js and ambari-web/vendor/scripts/bootstrap-tooltip.js.

zRains avatar May 13 '24 06:05 zRains

@zRains I have upgraded the Backbone to latest version now. Can you please recheck once?

vanshuhassija avatar May 15 '24 22:05 vanshuhassija

@vanshuhassija Your PR still has some issues, and I'm not sure if they only occur when using Yarn separately. Here are some notable problems:

  1. backbone.js isn't placed in the vendor folder. According to the configuration in the brunch file 'javascripts/vendor.js': /^vendor/, it won't be included in the final build result(vendor.js), leading to the Uncaught ReferenceError: Backbone is not defined error still occurring.

  2. The issue of vendor/scripts/bootstrap-popover.js and vendor/scripts/bootstrap-tooltip.js having identical content persists. Is this in reference to bootstrap-popover and bootstrap-tooltip?

  3. There's a misalignment in the UI after running:

image image

You can build and preview it follow the ambari-web/pom.xml and serve public folder by vite, my vite.config.js:

import { defineConfig } from 'vite'

export default defineConfig({
  server: {
    proxy: {
      '/api': {
        target: 'http://Your server addr',
        changeOrigin: true
      },
      '/api/stomp/v1/websocket': {
        target: 'ws://Your server addr',
        changeOrigin: true,
        ws: true
      }
    }
  }
})

zRains avatar May 16 '24 08:05 zRains

Can the front-end upgrade the charts to the latest ones? The old charts have outdated colors and can support more graphics, making page customization more flexible

smileyboy2019 avatar Jun 07 '24 07:06 smileyboy2019

@smileyboy2019 Upgrading the charts should be done after upgrading jQuery and other basic dependencies. IMO, both Node ^4.0 and Brunch.js are no longer maintained. It need to upgrade or replace dev/dependences, or, it will spend a significant amount of time dealing with compatibility and dependency issues.

zRains avatar Jun 12 '24 01:06 zRains

@vanshuhassija Could you please split this commit into two PRs(Jquery upgrade/Bootstrap upgrade)? It would be very helpful for the code review.

zRains avatar Jun 12 '24 01:06 zRains

@zRains The libraries are tightly coupled with each other. Upgrade of jquery would require upgrade of bootstrap as with the newer version of jquery, old bootstrap is not supported.

vanshuhassija avatar Aug 02 '24 04:08 vanshuhassija

@zRains The testing team in the organisation has pointed out some UI deviations as well which will be raised as tickets on the board once this PR is merged.

vanshuhassija avatar Aug 02 '24 04:08 vanshuhassija

@vanshuhassija Could u resolve conflict?

JiaLiangC avatar Aug 02 '24 06:08 JiaLiangC

@vanshuhassija Could you please resolve this conflict first? After that, other community members can continue to push forward with the work on upgrading the frontend components.

JiaLiangC avatar Aug 07 '24 01:08 JiaLiangC

@JiaLiangC The PR has no conflicts with the base branch. Any specific conflict you are referring to?

vanshuhassija avatar Aug 13 '24 20:08 vanshuhassija

@JiaLiangC The PR has no conflicts with the base branch. Any specific conflict you are referring to?

@vanshuhassija I have already manually resolved the conflict.

JiaLiangC avatar Aug 14 '24 00:08 JiaLiangC

@vanshuhassija Regarding this PR, I have concerns because it involves extensive changes. After applying this PR in the test environment, I noticed several bugs in the frontend. I'm worried that if we merge it, the community might not have enough frontend developers and contributors to address these issues. This could lead to many bugs remaining unresolved before the release. That's my main concern.

JiaLiangC avatar Aug 15 '24 01:08 JiaLiangC

Hi @vanshuhassija ,

thanks for reporting this and working on this which is much awaited one.

Can you please resolve the commits.? Changes are looks to good to me apart from the concerns raised already here.

As this was gone through the cycles of review, I am thinking we can commit now and raise the follow up jira's.

@JiaLiangC and @zRains any thoughts on this.?

brahmareddybattula avatar Sep 20 '24 14:09 brahmareddybattula

@brahmareddybattula resolved the conflicts

vanshuhassija avatar Sep 20 '24 15:09 vanshuhassija

@vanshuhassija Thank-you for providing fix for JQuery and Bootstrap upgrade fixes seems to have been long pending. changes look good to me +1 can you check the PR build failure ?

vishalsuvagia avatar Sep 22 '24 17:09 vishalsuvagia

@brahmareddybattula Sounds good. The main issue seems to be that the test cases for ambari-web have not yet been modified (144 failed test cases). This can be addressed gradually.

zRains avatar Sep 23 '24 04:09 zRains

@vanshuhassija Please fix the CI failure. After it's fixed, I'll merge it. As @zRains mentioned, any remaining issues will be addressed in future PRs.

JiaLiangC avatar Sep 24 '24 00:09 JiaLiangC

@JiaLiangC @brahmareddybattula @zRains @vishalsuvagia CI is now passing. Had to skip some test cases because of the known issues to be addressed in upcoming PRs. The test skip changes will be reverted with the upcoming fixes.

vanshuhassija avatar Sep 25 '24 07:09 vanshuhassija

@JiaLiangC @brahmareddybattula @zRains @vishalsuvagia CI is now passing. Had to skip some test cases because of the known issues to be addressed in upcoming PRs. The test skip changes will be reverted with the upcoming fixes.

Thanks for your excellent work!

JiaLiangC avatar Sep 25 '24 07:09 JiaLiangC

@JiaLiangC

LGTM. As discussed above, if there are no other objections, it will be merged in 24 hours. Any potential issues will be addressed in subsequent PRs.

@JiaLiangC Thanks

vanshuhassija avatar Sep 25 '24 09:09 vanshuhassija

Can you help me take a look at this issue? It seems to be a front-end problem。 (https://issues.apache.org/jira/projects/AMBARI/issues/AMBARI-26148?filter=allissues)

LiJie20190102 avatar Sep 29 '24 09:09 LiJie20190102

@brahmareddybattula @vanshuhassija thi pr has introduced almost 30 bugs, many of which are critical, causing major functionalities to break and disrupting most of the unit tests. They haven't been fixed yet. Here is the bug list:https://issues.apache.org/jira/browse/AMBARI-26237. Could you take a look, and help fixing them?

JiaLiangC avatar Dec 05 '24 03:12 JiaLiangC