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

bump to 2.25.1 #91

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

bump to 2.25.1 #91

wants to merge 2 commits into from

Conversation

ocefpaf
Copy link
Collaborator

@ocefpaf ocefpaf commented Sep 11, 2024

@srstsavage not sure if I got it right. Feel free to close it if you want to do this in a different way and/or later.

I'd love to address #90 in this PR. just not sure the best way to implement that.

2.25 is marked as a pre-release, let's keep it a draft for now.

Closes #94 and #95.

@ocefpaf ocefpaf marked this pull request as draft September 11, 2024 19:07
@callumrollo
Copy link
Contributor

2.25 official release today!

@ocefpaf
Copy link
Collaborator Author

ocefpaf commented Nov 1, 2024

@callumrollo, @abkfenris any idea of what is happening here? I never updated this before.

@abkfenris
Copy link
Collaborator

It seems like it's forgetting ${BASE_IMAGE} once another FROM is set. Try moving that ARG below the tomcat image FROM.

It feels a bit like a hiccup with the version of Docker though.

@srstsavage
Copy link
Member

@ocefpaf Arg, it's because the variables defining the base image names are defined as repository variables, which aren't set in your fork 🤦. Will have to rethink how that part works since forks should also build successfully.

As a temp workaround, you can set those vars:

https://github.com/ocefpaf/docker-erddap/settings/variables/actions

image

TOMCAT_AMD64_IMAGE = tomcat:10.1.26-jdk21-temurin-jammy@sha256:18952effb643bf192799e4ab2ca7c121d58871e96e5709fb5d405f4682a9aae7

TOMCAT_ARM64_IMAGE = tomcat:10.1.26-jdk21-temurin-jammy@sha256:4775c2227f16ee2726a35a1e97f43bfcb1a085cad23e169b1066ec9603826d8b

@srstsavage
Copy link
Member

Since we have an official ERDDAP docker image in the works I think #90 should be resolved there. Added a comment to discuss.

@@ -39,7 +39,7 @@ COPY --from=unidata-tomcat-image ${CATALINA_HOME}/conf/web.xml ${CATALINA_HOME}/
# Security enhanced server.xml
COPY --from=unidata-tomcat-image ${CATALINA_HOME}/conf/server.xml ${CATALINA_HOME}/conf/

ARG ERDDAP_VERSION=2.24
ARG ERDDAP_VERSION=2.25.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@srstsavage I believe you probably want to skip 2.25, right?

@ocefpaf
Copy link
Collaborator Author

ocefpaf commented Nov 8, 2024

Looks like Chris John is OK with the addition of a "docker" string int footer version string:

ERDDAP/erddap#225 (comment)

I'm not sure what is the best way to implement that though as I'm not that familiar ERDDAP to do this.

Dockerfile Outdated Show resolved Hide resolved
@ocefpaf ocefpaf force-pushed the bump_2.25 branch 4 times, most recently from 3afca95 to 6495841 Compare November 11, 2024 12:38
@@ -11,6 +11,8 @@ on:
env:
BUILDX_CACHE: /tmp/.buildx-cache
CACHE_KEY: docker-erddap-buildx-
TOMCAT_AMD64_IMAGE: tomcat:10.1.26-jdk21-temurin-jammy@sha256:18952effb643bf192799e4ab2ca7c121d58871e96e5709fb5d405f4682a9aae7
TOMCAT_ARM64_IMAGE: tomcat:10.1.26-jdk21-temurin-jammy@sha256:4775c2227f16ee2726a35a1e97f43bfcb1a085cad23e169b1066ec9603826d8b
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@srstsavage should I revert this and let someone else submit this PR without a fork? OK is this OK for now?

@ocefpaf ocefpaf marked this pull request as ready for review November 11, 2024 12:40
@ocefpaf ocefpaf changed the title bump to 2.25 bump to 2.25.1 Nov 14, 2024
@ocefpaf
Copy link
Collaborator Author

ocefpaf commented Nov 14, 2024

@srstsavage I'm not sure what is happening here now that I added the env vars It seems to be stuck :-/

If you want, feel free to close this one and re-do it later. Let's do whatever is easier to get the latest ERDDAP into the image.

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.

Update ERDDAP to v2.25
4 participants