-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
HBASE-28832 Upgrade from bootstrap 3.4.1 to non vulnerable version 5.3.3 #6490
base: master
Are you sure you want to change the base?
Conversation
df88b62
to
f2c49c1
Compare
<script src="/static/js/jquery.min.js" type="text/javascript"></script> | ||
<script src="/static/js/jquery.tablesorter.min.js" type="text/javascript"></script> | ||
<script src="/static/js/bootstrap.min.js" type="text/javascript"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I removed these JQuery and Bootstrap JavaScript includes because footer.jsp is included just above and that includes these already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I replaced hbase_logo_small.png under thrift module with the hbase-server one. Reason: to make sure that they are the same size (this under thrift was smaller).
If required this can be undone but I think it is more consistent this way.
This comment has been minimized.
This comment has been minimized.
Thanks for working on this @PDavid. Will try to build code locally and review the changes once the PR is ready. |
Many thanks @NihalJain ! I'll wait for the CI build and then I'll change this PR to ready to review. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The |
Hi @NihalJain, |
Ah yes, caught up at work, will review as soon as I have some bandwidth. |
Hi @PDavid I checked out code and had a look at Master and RS pages. All pages seem to work perfectly fine in these 2 web UIs, that's great work, thanks :) Have a few suggestions though
Can we make the UI coloring look similar to what we had before? For example now the blue colors are very dark over the dull blue we had earlier, which was not very heavy on eyes. You could try to override default bootstrap.css in hbase.css? For example replacing reference to #0d6efd with #337ab7 results in following. Would need to compare with old HBase UI where ever we have noticeable discrepancy in colors. Although it should be fine if new colors looks better than before.
Also the active navbar highlighting is either broken or not as before. Can this be fixed? This is how it used to render earlier BTW I have not looked at code changes line by line as of yet, have just tried the change locally. Will review code and other web pages soon. Overall things look good to me visually. |
Hi @NihalJain, Many thanks for the feedback! 👍 To be completely honest I was not sure how important it is to preserve the look and styling of the current HBase UI. Of course I'll' look into it and restore these. |
Thats right. As long as it looks visually same/better we are good. Blue color seemed too dark to me, and as for active bar indicator maybe we can just bold the text? currently it is not highligted appropriately. Need not try to replicate how it looked before. That would be an over kill. |
aefcd2d
to
d8541cf
Compare
Thanks @PDavid. Let me do another round of testing and come back to you soon. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
import="org.apache.hadoop.hbase.RegionMetrics" | ||
import="org.apache.hadoop.hbase.RegionMetricsBuilder" | ||
import="org.apache.hadoop.hbase.ServerMetrics" | ||
import="org.apache.hadoop.hbase.ServerName" | ||
import="org.apache.hadoop.hbase.Size" | ||
import="org.apache.hadoop.hbase.TableName" | ||
import="org.apache.hadoop.hbase.TableNotFoundException" | ||
import="org.apache.hadoop.hbase.client.AsyncAdmin" | ||
import="org.apache.hadoop.hbase.client.AsyncConnection" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the removed imports were redundant right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. These were unused imports which were highlighted by IntelliJ.
Hi @PDavid I have reviewed the updated pages and also had a look at REST and THRIFT pages. All looks good to me. Just a NIT: Can we fix this? I see two lines in all table boundaries. For eg: Can be fixed by replacing with Not important though, you can decide if you would want to fix. |
Also @PDavid please ensure to sync JIRA title and PR title |
My front end experience is minimal, so I'm not really qualified to review this. |
Thanks, I changed the PR title to be the same as the Jira title. |
Reason: to make sure that they are the same size (this under thrift was smaller).
This is the latest non-vulnerable version.
- Navbars now require a container within (to drastically simplify spacing requirements and CSS required). - The .active class can no longer be applied to .nav-items, it must be applied directly on .nav-links.
Data attributes for all Bootstrap JavaScript plugins are now namespaced to help distinguish Bootstrap functionality from third parties and your own code. For example, we use data-bs-toggle instead of data-toggle.
Breaking change in Bootstrap 5: Dropped form-specific layout classes for our grid system. Use our grid and utilities instead of .form-group, .form-row, or .form-inline.
to make the UI coloring look similar to what we had before.
to make sure the current active navbar link is highlighted appropriately.
(used by the "Regions in Transition" section on master status page) so that the paginator links are styled and displayed properly.
Before the tab content has its own bottom border and table row also had its own bottom border. This resulted in having two lines in all table boundaries. Fix is to remove the bottom border of the table row.
Make sure background color is visible in tables (Tasks) where alert class is used.
da71040
to
97972c8
Compare
With more testing I found some smaller issues which I now fixed and just pushed. I think this is ready now. |
So that buttons and textfields are displayed properly. Also removes unneeded inline styles.
97972c8
to
bbe00c0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Hi @PDavid thanks for all the backports, would be really helpful if you could mention the why the diffs are substantially smaller in branch-2.x vs master, a brief summary should suffice mentioning any extra/missing changes worth reviewing over there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skimmed through. Looks like the annotations for new bootstrap are more verbose than old. A lot of change for no changes. I'll defer to @NihalJain for fine-tuning, otherwise this looks good to me.
For your question on the mailing list, I think you're headed in the right direction.
Thanks a lot for taking on both points @PDavid .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, nice :)
Hi @NihalJain, Sure thing, thanks.
So because the web code was different, when I cherry-picked my commits I had to adapt the changes. I hope this answers your question. :) |
@PDavid if you can identify specific JIRAs that need to be backported, i find that it's better/easier to do that work than to maintain and enhance divergence between the branches. If you're up for it, can you backport those missing changes and rebase your branch-2 changes here on top of those backports? |
Hi @ndimiduk,
I'll look for the other differences. |
Hi @ndimiduk and @NihalJain,
What do you think? Is the approach good or should we have one PR per patch? |
You can present a single PR for all the backports for a particular branch. However, our process requires one commit per JIRA. |
Thank you @PDavid for the effort. Will have a look at the changes and reply back soon. Will hold merging this for now so that we can revisit/rebase backport of this PR once the new PRs for each branch is merged. Hopefully backports will be a lot similar post the missing JIRAs commit. FYI @apurtell @Apache9: I hope you are fine with backporting missing JIRAs to active 2.x branches with #6542, #6546 and #6548 |
It helps, if you turn off whitespace changes in the diff view and when you can check the changes by commits.
Details
First I upgraded to Bootstrap v4 - this was the majority of the changes - and then I upgraded to Bootstrap 5.3.3 - which is the latest non-vulnerable version according to https://security.snyk.io/package/npm/bootstrap/3.4.1
Changes were based on the migration documentation:
Most of the changes are related to navbar-s, tabs and forms.
The new Bootstrap look has a bit bigger font sizes but it is still OK in my opinion.
Bootstrap v5 is not requiring JQuery anymore but we have some JQuery plugins (jquery.tablesorter.min.js, tab.js) which still need it so I left JQuery in.
Changes
Screen shots
Master UI
Region Server UI
Thrift UI