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

Migrate the OSPool token issuer to the OSG image build repo #83

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bbockelm
Copy link
Contributor

No description provided.

Copy link
Contributor

@matyasselmeci matyasselmeci left a comment

Choose a reason for hiding this comment

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

I have a few suggestions, and also there are some files without newlines at the end, but the parts I can understand (which is to say not the tomcat or scitokens parts) are fine.

@@ -0,0 +1,99 @@
FROM hub.opensciencegrid.org/opensciencegrid/software-base:3.6-el8-release
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FROM hub.opensciencegrid.org/opensciencegrid/software-base:3.6-el8-release
FROM hub.opensciencegrid.org/opensciencegrid/software-base:3.6-al8-release

Can you try Alma 8? We're eventually going to replace the CentOS Stream 8-based software-base with Alma 8-based (as per SOFTWARE-5307) so it would be nice if we had one less thing to test before we transitioned.

<entry key="creation_ts">2022-01-19T19:55:55.172Z</entry>
<entry key="df_lifetime">-1</entry>
<entry key="scopes">["openid", "wlcg"]</entry>
<entry key="email">[email protected]</entry>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<entry key="email">gaynor@illinois.edu</entry>
<entry key="email">[email protected].edu</entry>

:-P

Seriously though -- according to the file name, this is some sort of template, so I don't actually know whose (or what's) email address this should contain...

@@ -0,0 +1,99 @@
FROM hub.opensciencegrid.org/opensciencegrid/software-base:3.6-el8-release

RUN yum install -y curl java-11-openjdk java-11-openjdk-devel
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RUN yum install -y curl java-11-openjdk java-11-openjdk-devel
RUN yum install -y curl java-11-openjdk java-11-openjdk-devel && \
yum clean all

a good habit to be in, although the space used by tomcat probably dwarfs the space from not clearing the yum cache.

Comment on lines +21 to +27
ARG TOMCAT_ADMIN_USERNAME=admin
ARG TOMCAT_ADMIN_PASSWORD=password
ADD tomcat-users.xml.tmpl /opt/tomcat/conf/tomcat-users.xml.tmpl
RUN sed s+TOMCAT_ADMIN_USERNAME+${TOMCAT_ADMIN_USERNAME}+g /opt/tomcat/conf/tomcat-users.xml.tmpl | sed s+TOMCAT_ADMIN_PASSWORD+${TOMCAT_ADMIN_PASSWORD}+g > /opt/tomcat/conf/tomcat-users.xml ;\
chgrp tomcat /opt/tomcat/conf/tomcat-users.xml

ARG TOMCAT_ADMIN_IP=127.0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This hurts my eyes. Can you please add a comment to the file explaining why baking the admin credentials into the image like this is OK?

@@ -0,0 +1 @@
java -jar /opt/scitokens-server/lib/jwt.jar -batch create_keys -single -o
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
java -jar /opt/scitokens-server/lib/jwt.jar -batch create_keys -single -o
#!/bin/sh
exec java -jar /opt/scitokens-server/lib/jwt.jar -batch create_keys -single -o

@@ -0,0 +1,37 @@
# Run the OA4MP command processor. This will allow you to edit, create or remove
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Run the OA4MP command processor. This will allow you to edit, create or remove
#!/bin/bash
# Run the OA4MP command processor. This will allow you to edit, create or remove

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