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

Second PR from feature-1481-java-17 #1735

Merged
merged 3 commits into from
Nov 15, 2023
Merged

Conversation

taojing2002
Copy link
Contributor

This is a simple PR:

  1. Removed two obsoleted properties which related to a SVN url in the metacat-common/pom.xml file.

  2. Changed the directory from tomcat8 to tomcat9 in the build.properties file.

…in the metacat-common/pom.xml file.

Change the directory from tomcat8 to tomcat9 in the build.properties file.
build.properties Outdated Show resolved Hide resolved
build.properties Outdated Show resolved Hide resolved
build.properties Outdated
@@ -8,12 +8,12 @@ metacat.version=3.0.0
metacat.releaseCandidate=

# Tomcat dev deployment directory
build.tomcat.dir=/usr/share/tomcat8
build.tomcat.dir=/usr/share/tomcat9
#build.tomcat.dir=/Users/leinfelder/tools/apache-tomcat-6
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

build.properties Outdated
#build.tomcat.dir=/Users/leinfelder/tools/apache-tomcat-6
# install ant target uses this to determine where to drop
# the war file in an installation. Test classes will use
# this value to find metacat.properties
app.deploy.dir=/opt/local/share/java/tomcat8/webapps
app.deploy.dir=/opt/local/share/java/tomcat9/webapps
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of having to continue editing this each time we upgrade tomcat, would it be feasible to just use a default directory name of tomcat instead of tomcat9? The user can then decide either to use a symlink, or to change this property.

My motivation: I didn't want to change this property because git would then always show it as modified (unless I add it to .gitignore, which could lead to other problems). I therefore added a symlink, which currently looks like this - not a great experience 🤣:

$ la /opt/local/share/java/
total 8
drwxr-xr-x  4 root  wheel  -  128B Sep 21 20:07 ./
drwxr-xr-x  3 root  wheel  -   96B Feb  8  2023 ../
-rw-r--r--  1 root  wheel  -  220B Sep 21 20:07 README.md
lrwxr-xr-x  1 root  wheel  -   34B Sep 21 20:04 tomcat8 -> /opt/homebrew/opt/tomcat@9/libexec

In case you're curious, here's what I wrote in README.md (I wrote it as a reminder after my build broke when I upgraded to tomcat 9):

$ cat /opt/local/share/java/README.md

# SHAMELESS HACK!

tomcat8 points to tomcat9 because tomcat8 is used in metacat's ant build.properties:

`app.deploy.dir=/opt/local/share/java/tomcat8/webapps`

...and I don't want to keep modifying it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no easy ways to accommodate different settings. The purpose of the build.properties files is to give users a way to customize their settings. So changing on this file is every common, just like the values.yam on k8s.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that. If it is common to edit this file, that also means we can choose any default value we want.

I therefore think it makes sense to choose a default value that is not tied to a particular version of tomcat. This will reduce maintenance overhead and potential mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. I will remove the version there.

@taojing2002 taojing2002 merged commit c4ad2af into develop Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants