METRON-1873: Update Bootstrap version in Management UI#1267
METRON-1873: Update Bootstrap version in Management UI#1267sardell wants to merge 13 commits intoapache:masterfrom
Conversation
|
Reviewed and tested. Seems good to me. +1 |
| "webpack-sources": "1.3.0", | ||
| "webpack-subresource-integrity": "1.1.0-rc.6" | ||
| "ajv": "~6.4.0", | ||
| "autoprefixer": "^8.4.1", |
There was a problem hiding this comment.
Are there any concerns around all of this stuff using "^", e.g. for avoiding licensing concerns if something changes license? I'm going to be entirely honest, my recollection of the exact details of the package-lock and the whole repeatable builds discussion with npm is hazy at best, so this question might just be rehashing that older discussion,
There was a problem hiding this comment.
When I first saw this, it caught me by surprise too. It turns out npm v6 made a change to the format of the package-lock.json file. Basically, dependencies in the lock file are fixed versions from what's listed in our project's package.json dependency section, whereas the requirements in the lock file are an exact copy of each package's dependency section from their package.json. In the end, it won't effect our ability to lock into specific versions of our project's npm dependencies.
It's a little confusing, but this thread (and specifically this comment) helped clear things up for me.
There was a problem hiding this comment.
Ahh, right, that's what the conclusion of that discussion was. Thanks for rehashing that (yet again).
| "font-awesome": "^4.6.3", | ||
| "jquery": "^3.3.1", | ||
| "karma-phantomjs-launcher": "^1.0.4", | ||
| "popper.js": "^1.14.4", |
There was a problem hiding this comment.
This is a new dependency for us, correct? Can you comment on the project license, size of the community backing this, etc? I am sure you have already considered these points, but we should have this discussion in the community for new dependencies.
There was a problem hiding this comment.
@nickwallen check out the contributor comments. It's MIT licensed and because bootstrap changed it's underlying library, not a choice @sardell had to make if my understanding is correct.
There was a problem hiding this comment.
Oops. Way ahead of me! Looks reasonable.
|
+1 Look good to me, pending @justinleet 's review. Thanks @sardell ! |
|
+1, thanks for taking care of this. |
4 participants
Contributor Comments
Link to original ticket here: https://jira.apache.org/jira/browse/METRON-1873
We are currently using an alpha version of Bootstrap v4. I updated the project to use the latest stable release (v4.1.3) for a couple of reasons. There were breaking changes between the alpha and stable release that make it impossible for us to use angular ports of Bootstrap like ng-bootstrap. In addition, there are quite a few features and fixes we would want in our project. The list is long, but if you're curious, [take a look at the changelog|https://github.com/twbs/bootstrap/releases].
Besides updating the bootstrap library, the work mainly consisted of updating deprecated class names and accounting for utility class value changes (for example, the class
.my-1would give an element 1 rem of margin on the y-axis using the alpha release, whereas it only gives .25rem using the stable releases).There was a dependency removed (tether.js) and one added (popper.js). This is because Bootstrap switched the library it uses for tooltips and dropdown menus with the update. Popper.js is licensed under the MIT license.
Testing
All unit tests pass as they should. Visual regressions can be tested manually or by using a diff tool like BackstopJs.
Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
For all changes:
For code changes:
N/A
Have you included steps to reproduce the behavior or problem that is being changed or addressed?Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
N/A
Have you written or updated unit tests and or integration tests to verify your changes?If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html:Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.