Closed sfreudenthaler closed 2 days ago
@wezell The addition of the "$@" after run was a change I made unnecessarily in the last commit. If you check the previous one it was not there before. As we had been setting the default CMD to "dotcms" I kept that and made it so that if you override with a different command it will run that instead. I wanted to make sure nothing broke if "dotcms" was set as the command specifically. even after discovering it had not had any effect as previously any command would do the same thing. Expecting this default from the dockerfile to always be set I did not set the behavior if not CMD was assigned, forgetting that the overriding dev docker file would do this. If you know where the "dotcms" parameter is otherwise used we should change this, otherwise setting "dotcms" or no command with this change will have the same impact of starting up the service otherwise any other command e.g. /bin/bash or even pg_dump will run without starting the server.
@sfreudenthaler @spbolton is line 21 correct?
exec -- ${TOMCAT_HOME}/bin/catalina.sh run "$@"
Do we need to force
dotcms
there?exec -- ${TOMCAT_HOME}/bin/catalina.sh run "dotcms"
I was able to start dotCMS with just catalina.sh run
when I was testing it last night locally. I know local is different than our full pipeline but it did seem to work.
I suspect the question is not if it's needed but if we should have it there.
I'm one hand it's kind of nice and more readable if it's there, you know by looking it's going to start dotCMS.
But, on the other hand it makes the code slightly more complicated to keep override ability. Assuming we'd need to set a default value in the case no args came with the call.
Issues
0 New issues
0 Fixed issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
No data about Duplication
Proposed Changes
s
typo outside of quoted string in console echoingdotcms
as the arg if no args are provided or if dotcms is the arg provideddotcms
as arg when invoking /srv/entrypoint.shChecklist
Additional Notes
This doesn't include the addition of test coverage. That will come in an additional PR once this is merged and working as expected.
Screenshots
REFS #28983
This PR fixes: #28983