CartoDB / CartoDB-SQL-API

CartoDB SQL API
BSD 3-Clause "New" or "Revised" License
62 stars 63 forks source link

build onprem image #678

Closed alberhander closed 3 years ago

alberhander commented 3 years ago
alberhander commented 3 years ago

Thanks a lot @thedae !!

@dgaubert I'm trying to add (at the bottom of config/environments/config.js the middleware configuration conditionally depending on a env var only used in onprem, but it doesn't seem to work. In case you can suggest me how to do that:

// load onprem licensing middleware
if (process.env.CARTO_SQL_ONPREM_MIDDLEWARE) {
  module.exports.routes.api[0].middlewares = require('/usr/src/app/onprem.js').middlewares
} else {
  module.exports.routes.api[0].middlewares = [
      function noop () {
          return function noopMiddleware (req, res, next) {
              next();
          }
      }
  ]
}
thedae commented 3 years ago

Thanks a lot @thedae !!

@dgaubert I'm trying to add (at the bottom of config/environments/config.js the middleware configuration conditionally depending on a env var only used in onprem, but it doesn't seem to work. In case you can suggest me how to do that:

// load onprem licensing middleware
if (process.env.CARTO_SQL_ONPREM_MIDDLEWARE) {
  module.exports.routes.api[0].middlewares = require('/usr/src/app/onprem.js').middlewares
} else {
  module.exports.routes.api[0].middlewares = [
      function noop () {
          return function noopMiddleware (req, res, next) {
              next();
          }
      }
  ]
}

@alberhander let me try this out, I'll get back to you with a proposal

dgaubert commented 3 years ago

It's a config file, why do you need to set configuration conditionally? I mean, you know when you are building an on-prem release. For me, it's another environment such as development, stagin, or production. We shouldn't set the configuration at runtime depending on the environment.

cc/ @alberhander

ilbambino commented 3 years ago

you are right with the idea that we are building it as a separate image. But what we wanted is to have one single config file and be able to inject the middleware only in the onprem case. So the idea was to set that env var in the build phase of the image, so it is "baked in" but without to maintain two config files. Because as it is an array with source code, injecting it through an env var is not so easy/clean. But if you have better ideas, we are all ears 👂

thedae commented 3 years ago

I have a proposal for this scenario: we could pass the prepended middlewares in an ENV var as a comma-separated ordered list of paths to those middlewares, for example:

middlewares: process.env.CUSTOM_MIDDLEWARES

With CUSTOM_MIDDLEWARES=/usr/src/app/onprem.js,/usr/src/app/another-one.js,/usr/src/app/cool-stuff.js

Generic solution that does not require to add specific onprem references in the config.js

This would require a small change in the piece of code where we evaluate this config entry, fairly easy to achieve.

What do you think @dgaubert @ilbambino @alberhander ?

ilbambino commented 3 years ago

I like the idea 👍

dgaubert commented 3 years ago

Go for it! In previous on-prem releases, we couldn't use env variables. If the new process to build on-prem allows them, your approach is wide better.

alberhander commented 3 years ago

We can ignore the dev-env tests failing, nothing to do with the sql-api

alberhander commented 3 years ago

Already tested the image in on-prem, and it works! :muscle:

alberhander commented 3 years ago

Tested again, and working as expected :+1: