Skip to content
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

website: restore pom.xml #15113

Closed
wants to merge 4 commits into from
Closed

website: restore pom.xml #15113

wants to merge 4 commits into from

Conversation

317brian
Copy link
Contributor

@317brian 317brian commented Oct 9, 2023

This restores the pom.xml file from right before the Docusaurus 2 upgrade. It's copied from the commit immediately prior to the upgrade from the 27.0 branch: https://github.com/apache/druid/blob/25e6e28e657767885f2126e1ec1e016c1d20e873/website/pom.xml

website/pom.xml Outdated
<parent>
<groupId>org.apache.druid</groupId>
<artifactId>druid</artifactId>
<version>27.0.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

this needs updated to 28.0.0-SNAPSHOT

Comment on lines +42 to +43
<node.version>v16.17.0</node.version>
<npm.version>8.15.0</npm.version>
Copy link
Member

Choose a reason for hiding this comment

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

are these the recommended docusaurus 2 versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't have a recommended NPM version and the recommended node version is 16.14 or later

@clintropolis clintropolis added this to the 28.0 milestone Oct 9, 2023
@317brian 317brian marked this pull request as draft October 9, 2023 21:19
@317brian
Copy link
Contributor Author

317brian commented Oct 9, 2023

Running mvn compile, mvn -pl . test within website works and builds output to website/build as expected.

<parent>
<groupId>org.apache.druid</groupId>
<artifactId>druid</artifactId>
<version>28.0.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

i guess this needs to be 29 snapshot now since #15121 has been merged

Copy link
Contributor Author

@317brian 317brian Oct 12, 2023

Choose a reason for hiding this comment

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

Was this used for anything outside of building the website and interpolating the version variable used in the docs?? The variable interpolation got moved to druid-website-src along with the rest of the stuff for a production build.

If not, we can probably delete it.

Copy link
Member

Choose a reason for hiding this comment

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

it needs to match the rest of the pom versions if we are keeping this file at all i think, https://github.com/apache/druid/blob/master/pom.xml#L2128 has a profile that refers to this file. It seems useful to me to have a pom for the website that can run basic npm build and test commands the same as we have for web-console if its going to live in the same repository as the rest of druid stuff. whether or not it needs to build the full website or not i suppose is a different matter, but i think at least it should be able to run npm build and run the spellcheck (so that it can be done independently of whatever node/npm versions exist on the developers machine globally)

Copy link
Member

Choose a reason for hiding this comment

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

for example https://github.com/apache/druid/blob/master/distribution/asf-release-process-guide.md#set-version-and-make-a-tag still refers to bumping the website version so it matches everything else. it does look like maybe this part has been updated at least https://github.com/apache/druid/blob/master/distribution/asf-release-process-guide.md#update-druidstagedapacheorg, having not built the full website i'm not sure if it would ever make sense to try to drive that process through here instead of having to drive from the website-src repo...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks for taking the time to explain it! I'm pretty unfamiliar with Maven since it's not really in the typical docs workflow.

I haven't had time to figure out why the spellchecker doesn't run when I do something like mvn compile though or if it runs, it doesn't return any feedback. Manually running it returns feedback on errors or success. The spellchecking script hasn't changed. The build runs and completes and we get feedback on it though

@LakshSingla
Copy link
Contributor

@317brian Is this PR still required or can we close this out?

@317brian
Copy link
Contributor Author

It's required but not tied to any particular release since we don't build the website with this repo anymore. The fix is a quality of life fix for contributors.

@clintropolis
Copy link
Member

we might want to consider removing the website profile from the 28 branch if we aren't going to merge and backport this

@abhishekagarwal87
Copy link
Contributor

If we need not build website with this repo, we could just remove the profile itself. The profile was added so we are bumping up the pom version in website module. That's no longer needed now.

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