Skip to content

FIREFLY-1648: fix Dockerfile parsing envVars bugs#1698

Merged
loitly merged 2 commits intodevfrom
FIREFLY-1648-envvars-parsing
Feb 5, 2025
Merged

FIREFLY-1648: fix Dockerfile parsing envVars bugs#1698
loitly merged 2 commits intodevfrom
FIREFLY-1648-envvars-parsing

Conversation

@loitly
Copy link
Contributor

@loitly loitly commented Feb 3, 2025

Ticket: https://jira.ipac.caltech.edu/browse/FIREFLY-1648

This PR addresses several issues and improvements related to environment variable parsing.

The key changes are:

  • Resolved issues with escaping special characters during Dockerfile parsing of environment variables.
  • Rewritten the old launchTomcat.sh launch script into entrypoint.py, which is now the primary entrypoint for launching Tomcat.
  • Add unit test entrypoint_test.py to make testing easier when modifying entrypoint.py
  • Expose System Properties in ServerStatus
  • Updated Jenkinsscripts to ensure that special characters in env variables are properly escaped

Test via Jenkins + Helm: https://firefly-1648-envvars-parsing.irsakudev.ipac.caltech.edu/irsaviewer/
This is built with FIREFLY_PROPS=

OP_tap_additional_services_0_label=IRSA
OP_tap_additional_services_0_value=https://irsadev.ipac.caltech.edu/TAP
OP_tap_additional_services_0_hipsUrl=ivo://CDS/P/SDSS9/color
OP_tap_additional_services_0_fovDeg=2
OP_tap_additional_services_0_centerWP=5;5;EQ_J2000

I'm not entirely sure what all of this means, but I can confirm that it's hitting irsadev instead of irsa and that HiPS coverage is using SDSS9 color.

Test via Docker Compose:

Added to firefly-docker.env

## FIREFLY-1648 tests  -- will remove before merging.
pathPrefix=local       # path to firefly is now /local/firefly/ instead of /firefly/
PROPS="abc=123;semicolon=abc;;xyz;space=abc xyz;quote=abc\"xyz\""
PROPS_OP_tap_additional_services_0_centerWP=5;5;EQ_J2000

Start Firefly using Docker Comopse

cd firefly
docker compose up firefly --build

Verify that Firefly is now running at http://localhost:8080/local/firefly/ instead of /firefly/
Go to /admin/status -> click 'Debug Info'
to see that PROPS (multi-props) and PROPS_OP_tap_additional.. (single prop) are set.

You can also run docker compose run firefly --dry-run to see what envVars were used and how it will appears in the CATALINA_OPTS envVar.

If you have python3 installed, you can run unit tests:

cd firefly/docker
python3 entrypoint_test.py

These are fixes to Jenkins script if interested. commit and commit

@loitly loitly requested a review from robyww February 3, 2025 21:38
@loitly loitly self-assigned this Feb 3, 2025
@robyww
Copy link
Contributor

robyww commented Feb 4, 2025

I want to look at this in detail but right away I think it is important to test Jenkin with a the first as

OP_tap_additional_services_0_label='IRSA dev with spaces'

I don't see spaces in you string test

also

OP_tap_additional_services_0_label='IRSA dev, with spaces and commas'

Copy link
Contributor

@robyww robyww left a comment

Choose a reason for hiding this comment

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

It looks good and appears to work. I think the python makes it easier to work with and to read.

I really like the parameter passing being so clean now.

If you want to keep launchTomcat.sh (it might a good idea for a cycle or two) around for awhile you should put some deprecation message at the top.

I do think if we are going to do this in python we should follow PEP 8. That is what we are doing with firefly_client. IntelliJ sets that up by default I think. I least mine does.

There are several sections that needs to be cleaned up to be PEP 8 like.

I personally am not a big fan of PEP 8 but I think we should be consistent.

Comment on lines +78 to +81
print("========== Information: you can set environment variable using -e on docker run line ===== \n")
print("Environment Variables:")
print(" Description Name Value")
print(" ----------- -------- -----")
Copy link
Contributor

Choose a reason for hiding this comment

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

You would do this whole thing with:

print(f"""
  ...many lines of stuff...
""")

Comment on lines +146 to +152
os.environ["JPDA_ADDRESS"] = "*:5050"
CATALINA_HOME = os.getenv("CATALINA_HOME", "/usr/local/tomcat")
VISUALIZE_FITS_SEARCH_PATH = os.getenv("VISUALIZE_FITS_SEARCH_PATH", "")
START_MODE = os.getenv("START_MODE", "run")
NAME = os.getenv("BUILD_TIME_NAME", "ipac/firefly")
ADMIN_USER = os.getenv("ADMIN_USER", "admin")
ADMIN_PASSWORD = os.getenv("ADMIN_PASSWORD", base64.b64encode(str(random.randint(100000, 999999)).encode()).decode()[:8])
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not very standard python style.

instead

catalina_home = os.getenv("CATALINA_HOME", "/usr/local/tomcat")

and other similar lines


return props_opts

def add_single_prop_env_vars():
Copy link
Contributor

Choose a reason for hiding this comment

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

PEP3 is picky... it wants to lines before a function

@loitly loitly merged commit 5e9f31b into dev Feb 5, 2025
@loitly loitly deleted the FIREFLY-1648-envvars-parsing branch February 5, 2025 23:14
@robyww robyww added this to the 2025.2 milestone Feb 6, 2025
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.

2 participants