-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29609, HBASE-29152 Checkstyle upgrade, site skin replacement #7353
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
Conversation
TestingCreated the site with Also made sure
|
<artifactItem> | ||
<groupId>org.webjars</groupId> | ||
<artifactId>bootstrap</artifactId> | ||
<version>2.3.2</version> | ||
<type>jar</type> | ||
<overWrite>true</overWrite> | ||
<includes>**/css/bootstrap-responsive.min.css</includes> | ||
</artifactItem> | ||
<artifactItem> | ||
<groupId>org.webjars</groupId> | ||
<artifactId>highlightjs</artifactId> | ||
<version>8.7</version> | ||
<type>jar</type> | ||
<overWrite>true</overWrite> | ||
<includes>**/styles/github.min.css, | ||
**/highlight.min.js,</includes> | ||
</artifactItem> |
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.
These are not required anymore as the new skin brings them from a separate jar.
<version>${maven-site.version}</version> | ||
<configuration> | ||
<siteDirectory>${basedir}/src/site</siteDirectory> | ||
<customBundle>${basedir}/src/site/custom/project-info-report.properties</customBundle> |
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.
There was a warning regarding this during the build:
[WARNING] Parameter 'customBundle' is unknown for plugin 'maven-site-plugin:3.12.0:site (default-site)'
Removing this line fixed it.
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.
Removed this because of the mentioned warning:
[WARNING] Parameter 'customBundle' is unknown for plugin 'maven-site-plugin:3.12.0:site (default-site)'
<!-- The ID of the Google custom search engine to use. | ||
This one searches hbase.apache.org, issues.apache.org/browse/HBASE-*, | ||
and user and dev mailing list archives. --> | ||
<customSearch>000385458301414556862:sq1bb0xugjg</customSearch> |
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 did not found a solution (yet) to use this Google custom search engine with the newer skin.
Do you think it is still needed? Because most probably this can be solved somehow but may require more work / hacks.
This comment has been minimized.
This comment has been minimized.
47958ba
to
c94d927
Compare
This comment has been minimized.
This comment has been minimized.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
I would split the skin replacement and the checkstyle update into separate tickets and PRs if possible. I also suggest starting a [DISCUSS] thread about the skin update to make sure that everyone is aware of this and has a chance to comment. |
Also, there is some information on why HBase needed the hacked theme in the old tickets. |
Thanks for the suggestion. Sure, actually, I was wondering about the same thing: to do these separately. However having them in the same PR proves that the new skin works with the new checkstyle version. But of course I can separate them. :)
Good idea, I'll do that.
I checked the commits on the 1.4 branch of the fork which were made in 2019, since a lot of time passed, even the original Maven fluido skin is now at 2.1.0 version and I did not found any issues with the new skin I thought the original problems might not be present anymore. But I'll look into this to be sure. |
Closing this PR as I'll raise two separate PR-s for the checkstyle upgrade and the skin replacement. |
OK, in the meantime I found out that I already started a [DISCUSS] thread about the skin update: I'll post a new message there. Also it seems HBase master does not uses Josh Elser's old patched skin because Duo changed it back to the official Maven-Fluido skin here: https://github.com/apache/hbase/pull/5993/files , just the comment about Josh's skin was left there. I opened #7355 for the skin update. |
Uh oh!
There was an error while loading. Please reload this page.