GEOS-11555: fix installation of extensions in various configurations #102

spike83 · 2024-10-01T06:47:10Z

With this PR, I want to address the issues GEOS-11555 and GEOS-11258 about some inabilities of the extension installer script.

In my opinion, it should be possible to add community modules also to official images. So, I added a warning message instead of blocking it.

Also, I tried to get rid of the confusion in the naming of the variable INSTALL_EXTENSIONS which was controlling the download behavior. Thus, I introduced a variable DOWNLOAD_EXTENSIONS which controls this and made the INSTALL_EXTENSIONS to really control the installation of extensions.

On one point, I'm not sure: Should we fail the startup when we are unable to download extensions?
On one hand, the full functionality might not he available and cannot be detected with any sort of health check and on the other hand, maybe it's better to run without an extension than not run at all.

reinout · 2024-10-01T07:16:42Z

README.md

+| COMMUNITY_PLUGIN_BASE_URL | Specifies the **base** URL for downloading the latest community-contributed GeoServer plugins. | `https://build.geoserver.org/geoserver/` | |
+| COMMUNITY_PLUGIN_URL | Specifies the URL for downloading the latest community-contributed GeoServer plugins | `${COMMUNITY_PLUGIN_BASE_URL}2.25.x/community-latest` |


I think it is clearer to end both URLs in the same way (with or without slash). Without slash is visually clearer (and matches the original COMMUNITY_PLUGIN_URL).

Suggested change

| COMMUNITY_PLUGIN_BASE_URL | Specifies the **base** URL for downloading the latest community-contributed GeoServer plugins. | `https://build.geoserver.org/geoserver/` | |

| COMMUNITY_PLUGIN_URL | Specifies the URL for downloading the latest community-contributed GeoServer plugins | `${COMMUNITY_PLUGIN_BASE_URL}2.25.x/community-latest` |

| COMMUNITY_PLUGIN_BASE_URL | Specifies the **base** URL for downloading the latest community-contributed GeoServer plugins. | `https://build.geoserver.org/geoserver` | |

| COMMUNITY_PLUGIN_URL | Specifies the URL for downloading the latest community-contributed GeoServer plugins | `${COMMUNITY_PLUGIN_BASE_URL}/2.25.x/community-latest` |

Although... I now notice that ADDITIONAL_LIBS_DIR seems to include the trailing slash. In that case, end both of these URLs with a slash?

reinout · 2024-10-01T07:18:31Z

README.md

-The following values cannot really be safely changed (as they are used to download extensions and community modules as the docker image first starts up).
-| VAR NAME | DESCRIPTION | SAMPLE VALUE |
-|--------------|-----------|------------|
+| WGET_OPTS | Options for the `wget` command | `--no-check-certificate` |


Wasn't it your intention to remove this --no-check-certificate? (Which is a good idea to remove in my opinion :-) )

Yes, it is removed by this PR, but there is the variable WGET_OPTS in case you would for some reason need to set some options for wget. By default, this is empty, just mentioning a sample value here.

install-extensions.sh

reinout · 2024-10-01T07:33:21Z

Sorry for the multiple comments, I should have done it properly as a single review. I thought I had only one quick suggestion :-)

Fixing up the if/else errors and the community plugin installs in the install script seems like a good idea.

One thing that worries me a bit are the extra environment variables. There are a lot already. Could your fix work without the extra environment variables?

DOWNLOAD_EXTENSIONS: if you specify an extension, it should be fine to download it if missing. Why the extra check?
COMMUNITY_PLUGIN_BASE_URL: a variable in the bash script is probably handy, but I don't think it needs to be configurable?

Co-authored-by: Reinout van Rees <reinout@vanrees.org>

spike83 · 2024-10-02T05:23:17Z

DOWNLOAD_EXTENSIONS is basically the functionality from the former INSTALL_EXTENSIONS variable to disable the download. This will prevent some errors when run in an environment where the geoserver has no internet access.

COMMUNITY_PLUGIN_BASE_URL allows changing the server for the build artifacts but keep the logic of the structure underneath it.

I BTW missed one ;-))

It could make sense to split them into multiple tables to get some clarity and better overview. Suggesting Geoserver, Tomcat, Extensions

jodygarnett · 2024-10-08T09:53:10Z

I would be very unhappy with installing community modules into a release intended for production - the point of them is to get feedback on snapshot / nightly build releases to support development. If people want to use extensions in production - stop messing around and fund / complete them.

Really we are already pushing our luck distributing these as nightly builds as they have not been given to the project steering committee for review and publication yet.

buehner

I finally found some time to have a look at this and added some first comments. Will try to get this running in a next step. But in general I really like these enhancements.

Could you already rebase your branch against master? There are conflicts currently...

buehner · 2024-11-20T14:28:30Z

BUILD.md

-  --build-arg COMMUNITY_PLUGIN_URL=https://build.geoserver.org/geoserver/2.25.x/community-latest/ \
-  --build-arg GS_VERSION=2.24-SNAPSHOT \
-  -t 2.25.x .
+  --build-arg WAR_ZIP_URL=https://build.geoserver.org/geoserver/2.27.x/geoserver-2.27.x-latest-war.zip \


These examples do not (yet) work as there is not (yet) a 2.27.x folder: See https://build.geoserver.org/geoserver

buehner · 2024-11-20T14:30:43Z

README.md

@@ -226,25 +240,25 @@ Following is the list of the all the environment variables that can be passed do
 | CATALINA_OPTS | Catalina options. Check [ref](https://www.baeldung.com/tomcat-catalina_opts-vs-java_opts) | `-Djava.awt.headless=true` |
 | GEOSERVER_DATA_DIR | Geoserver data directory location | `/opt/geoserver_data/` |
 | GEOSERVER_REQUIRE_FILE | Geoserver configuration used interally | `/opt/geoserver_data/global.xml` |
-| INSTALL_EXTENSIONS | Indicates whether additional GeoServer extensions should be installed | `false` |
+| DOWNLOAD_EXTENSIONS | Indicates whether additional GeoServer extensions should be installed | `false` |
+| INSTALL_EXTENSIONS | Indicates whether additional GeoServer extensions should be installed | `true` |


The description of DOWNLOAD_EXTENSIONS is exactly the same as for INSTALL_EXTENSIONS. I guess this was not adjusted after copy and pasting? ;-)

buehner · 2024-11-20T14:49:29Z

install-extensions.sh

+      COMMUNITY_EXTENSIONS_VERSION="${GEOSERVER_VERSION/-SNAPSHOT/.x}"
+      COMMUNITY_EXTENSIONS_VERSION="${COMMUNITY_EXTENSIONS_VERSION%.*}-SNAPSHOT"
+    fi
+    COMMUNITY_PLUGIN_BASE_URL=${COMMUNITY_PLUGIN_BASE_URL:-"https://build.geoserver.org/geoserver"}


Unlike others (like COMMUNITY_PLUGIN_URL) this new env COMMUNITY_PLUGIN_BASE_URL is not declared in the Dockerfile. I think it makes sense to also add it there for the sake of completeness (and to avoid confusion)

buehner · 2024-11-20T14:51:39Z

install-extensions.sh

+      GEOSERVER_RELEASE_BRANCH="${GEOSERVER_RELEASE_BRANCH%.*}.x"
+
+    fi
+    COMMUNITY_PLUGIN_URL=${COMMUNITY_PLUGIN_URL:-"${COMMUNITY_PLUGIN_BASE_URL}/${GEOSERVER_RELEASE_BRANCH}/community-latest/"}


In general: wouldn't it make sense to set the meaningful/working defaults for COMMUNITY_PLUGIN_BASE_URL and COMMUNITY_PLUGIN_URL in the Dockerfile, so we would not have to set them here?

buehner · 2024-11-20T15:01:41Z

I would be very unhappy with installing community modules into a release intended for production

Maybe this is a misunderstanding? I don't think there is any intention to build community plugins into images. One intention of this PR is to optionally allow users to include experimental community plugins on startup/boot time of the container

jodygarnett · 2024-11-20T19:45:57Z

I would be very unhappy with installing community modules into a release intended for production

Maybe this is a misunderstanding? I don't think there is any intention to build community plugins into images. One intention of this PR is to optionally allow users to include experimental community plugins on startup/boot time of the container

Yes - but only when testing a nightly build please (we do not want to mix a release image with community plugins).

aside: I am considering a proposal to change the policy and make a distribution that includes community plugins as a "tech preview" or something. But until that time we do not have permission to distribute this stuff for production - instead they are shared with the public by the individual developers seeking support/funding.

When ready the developer will provide to geoserver project for IP review and distribution as part of GeoServer.

aaime · 2024-11-26T07:36:27Z

I would be very unhappy with installing community modules into a release intended for production - the point of them is to get feedback on snapshot / nightly build releases to support development.

This position is your personal ideology Jody, but has nothing to do with the GeoServer project at a large. Since community module existed, users have been free to install community modules in releases as they pleased, in their bin, win installers and war packages.

In the docker image as it stands, an artificial limitation has been added. My suggestion is simple: stop treating users as babies. Mixing release and community modules can result in a GeoServer not starting any longer, it is possible and it has actually happened... but as long as the users are made aware of the risks (stick it in bold font in the docs) it's fine. No need to be patronizing.

GEOS-11555: fix installation of extensions in various configurations

0a0b152

reinout reviewed Oct 1, 2024

View reviewed changes

install-extensions.sh Outdated Show resolved Hide resolved

spike83 and others added 2 commits October 2, 2024 06:57

Update install-extensions.sh

739b795

Co-authored-by: Reinout van Rees <reinout@vanrees.org>

GEOS-11555: fix slashes and docs

1186c57

buehner reviewed Nov 20, 2024

View reviewed changes

buehner mentioned this pull request Nov 20, 2024

fixed community extension URL #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEOS-11555: fix installation of extensions in various configurations #102

GEOS-11555: fix installation of extensions in various configurations #102

spike83 commented Oct 1, 2024

reinout Oct 1, 2024

reinout Oct 1, 2024

reinout Oct 1, 2024

spike83 Oct 2, 2024

reinout commented Oct 1, 2024

spike83 commented Oct 2, 2024

jodygarnett commented Oct 8, 2024

buehner left a comment

buehner Nov 20, 2024

buehner Nov 20, 2024

buehner Nov 20, 2024

buehner Nov 20, 2024

buehner commented Nov 20, 2024

jodygarnett commented Nov 20, 2024 •

edited

Loading

aaime commented Nov 26, 2024

		\| COMMUNITY_PLUGIN_BASE_URL \| Specifies the base URL for downloading the latest community-contributed GeoServer plugins. \| `https://build.geoserver.org/geoserver/` \| \|
		\| COMMUNITY_PLUGIN_URL \| Specifies the URL for downloading the latest community-contributed GeoServer plugins \| `${COMMUNITY_PLUGIN_BASE_URL}2.25.x/community-latest` \|

GEOS-11555: fix installation of extensions in various configurations #102

Are you sure you want to change the base?

GEOS-11555: fix installation of extensions in various configurations #102

Conversation

spike83 commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reinout commented Oct 1, 2024

spike83 commented Oct 2, 2024

jodygarnett commented Oct 8, 2024

buehner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buehner commented Nov 20, 2024

jodygarnett commented Nov 20, 2024 • edited Loading

aaime commented Nov 26, 2024

jodygarnett commented Nov 20, 2024 •

edited

Loading