Updated build process

[jonrandahl]

Resolved incorrect traps for missing env vars in entrypoint.sh, added updated commands for Dockerfile, as well as added new SHORTNAME variable to Makefile.

[andrew-pickin-epi]

What is the point of setting SCRIPT_NAME via this environment variable?
This can only break the system.

[jonrandahl]

@andrew-pickin-epi SCRIPT_NAME is used to "[... ] have both the assets paths and path helpers configured for serving a Rails application from a subdirectory." ¹

It is similar to RAILS_RELATIVE_URL_ROOT in that "[...] SCRIPT_NAME isn't available during the asset compilation task so config.relative_url_root needs to be set so that it knows where they will be served from by the webserver. This is especially important for Sass since the asset helpers like image-url, etc. will generate absolute urls." ²

I don't follow how this can break the system though and we went through all this together?

Footnotes

1. https://jlintusaari.net/deploying-rails-application-to-subdirectory/ ↩

2. https://github.com/rails/rails/issues/8119#issuecomment-25442568 ↩

[andrew-pickin-epi]

You can break the system is RAILS_RELATIVE_URL_ROOT different at run-time from what it's set to at build-time (or if either of these is different from SCRIPT_NAME). And this is still possible,

[andrew-pickin-epi]

APPLICATION_ROOT is stale here.
API_SERVICE is redundant here.

[jonrandahl]

This echo was created as a verification flag for the build process and, while I agree and will remove the APPLICATION_ROOT variable, having the API_SERVICE_URL is good to know at this point in case you're needing to validate any local configuration being passed in at invocation.

[jonrandahl]

@andrew-pickin-epi Upon further reflection I've not removed the APPLICATION_ROOT from the application as it does allow us to pass in a temporary value, like /, when trying to get things running locally.

I would like to say I think this makes the app more robust when developing locally even as an image as that will also match the development.rb configuration settings:

  # The RAILS_RELATIVE_URL_ROOT should be set in the entrypoint.sh script via the 
  # build-time environment variable `APPLICATION_ROOT`; however, if this is not set
  # the following fallback is passed in as a standard root value when the app is run
  # directly via `rails server`
  config.relative_url_root = ENV.fetch('RAILS_RELATIVE_URL_ROOT', '/')

Therefore the echo above will allow us to verify the expected value is being used when creating and/or running the image.

N.B. Please forgive me if I've gotten my build-time v. run-time variables mixed up nonetheless! 🙏

[andrew-pickin-epi]

It doesn't make it more robust.
If it's possible to set the variable differnetly at build time and run time the it can break,
IF you want to have two setups to run with the without a path then define different environment files.
Further this IS a function of rails or at least rake. So No APPLICAITON_ROOT makes no sense at all.

[jonrandahl]

@andrew-pickin-epi while trying to resolve the proxy configuration mentioned previously via further investigation of the older versions of this application, and therefore also the other LR app configurations, we seem to have gotten things slightly muddled up in our approach.

While I appreciate your guidance throughout these updates, and a lot of good choices were made, I would like to revert the RAILS_RELATIVE_URL_ROOT run-time variables to it's previous configuration based on my findings; let me explain.

Back in January we discussed and agreed that the naming of the variables being passed around during the build process, specifically RAILS_RELATIVE_URL_ROOT, was misleading in its use in the build files. We decided then to rename that specific variable to APPLICATION_PATH APPLICATION_ROOT to be more clear on its purpose.

I, in my Rails naivety, misunderstood the use for that specific name and changed all instances of the RAILS/Rake variable used across the application as well.

@der kindly highlighted my error to which I revised the renamed variable to only be the ones used in the build files, to which you then updated the validation methods you felt were appropriate in the entrypoint.sh file.

Upon further investigation, and in order to improve the quality of the codebase for the new infrastructure, we revised those validation flags together whilst also realising that the run-time value would work if the variable was "locked" to the expected /app/root path value in the rails production configuration file. We then cleaned up those validations to only check for the API_SERVICE_URL where applicable (funny enough, not in this application).

The Makefile configuration was also refactored as we realised the value being passed in during the precompile phase was being overwritten by the run-time locked value and therefore unnecessary.

While investigating the original configuration of the build files [52, 72, 79] to sort out the proxy I've come to the conclusion that APPLICATION_ROOT, and subsequently RAILS_RELATIVE_URL_ROOT, should be available to the build process on ALL environments because if you're trying to run the docker image locally without having the proxy running you cannot see the styles, scripting, etc. if you can't change the RAILS_RELATIVE_URL_ROOT value; and if you try to run the rails server outside of docker in production mode to use the locked value, it won't build because the RAILS_SECRET isn't available.

Could we please discuss this together to agree on an approach that will allow for the most application resilience while still allowing for all of the environments to be run, tested, and validated both inside and out of docker images?

PS. I'm not averse to introducing specific environment files as suggested if that's the best way forward?

[andrew-pickin-epi]

What is PUBLIC_NAME?
RAILS_RELATIVE_URL_ROOT and SCRIPT_NAME should not be used.

[der]

PUBLIC_NAME is set on line 9 (to LR Landing) presumably to give one place to change a bit of text that's embedded in two locations.

Not understanding the objection to logging the value of RAILS_RELATIVE_URL_ROOT to aid debugging but agreed that SCRIPT_NAME is redundant given how it's being set.

[jonrandahl]

I added PUBLIC_NAME to allow better code portability and is similar to SHORTNAME in the Makefile; which is used to flag which app is being initiated.

Again, the echo itself was another verification flag for the build process and should be left as is.

I could wrap this in a check for RAILS_ENV being development if that helps?

[andrew-pickin-epi]

Env var table needs re-write.

[jonrandahl]

I'm working on unpicking the comments and threads found on epimorphics/hmlr-linked-data#52 in order to resolve epimorphics/hmlr-linked-data#79 and your comment.

[andrew-pickin-epi]

This is inconsistent with ports in production.
Move proxy from 3001.

[jonrandahl]

I agree, I used localhost:3030 in my changes which I will push up in a new PR for the simple web proxy repository and update accordingly where needed.

[andrew-pickin-epi]

I've reconsidered this. I think the proxy should listen on 8080.

[jonrandahl]

The issue with using 8080 is that's the API port: http://localhost:8080 set in the API_SERVICE_URL default value?

Mind you I only went with 3030 as it was still in the 3k range but we could choose something that works for all the different endpoints?

[der]

Can't really comment the SCRIPT_NAME discussion. Have attempted to understand where that might be used without success. If it's not actually needed then better to get rid of it but if testing shows it is need then so be it.

Once that's resolved then happy for this to merge.

[der]

PUBLIC_NAME is set on line 9 (to LR Landing) presumably to give one place to change a bit of text that's embedded in two locations.

Not understanding the objection to logging the value of RAILS_RELATIVE_URL_ROOT to aid debugging but agreed that SCRIPT_NAME is redundant given how it's being set.

[andrew-pickin-epi]

Does landing work with RAIL_RELATIVE_URL_ROOT work with anything but '/'.
Is this machinery and log entry relevant for Landing?

[jonrandahl]

@andrew-pickin-epi this was felt to be logical and safe if a RAILS_RELATIVE_URL_ROOT value was passed in we would report the non-default value

[andrew-pickin-epi]

Why are you referencing API_SERVICE_URL in this repository when it's not used here.
Further the, where it is being used we should attempt to use the internal docker name as reference in the Makefile, unless there is a reason to used something else when attempting to set up some alternative routing, in which case the purpose of the alternative should be made clear.

[jonrandahl]

Andy, when you're running locally in Rails and you want to test Qonsole then you need to pass in the API_SERVICE_URL or else Qonsole doesn't have the api access needed as per the comment above states.

[andrew-pickin-epi]

There is a section here call ed "Developer notes" but the contents are not just for a audience of developers, this is a bad choice of title