commercetools / commercetools-paypal-plus-integration

Integration between commercetools API and PayPal Plus API
Apache License 2.0
7 stars 1 forks source link

Add service configuration loading and mapping #3

Closed floriansattler closed 7 years ago

floriansattler commented 7 years ago

The config will be smth like this:

YAML example

tenants=bulls,pegasus
tenant.bulls:
  ctp.client:
    projectKey: blah-blah-bulls
    clientId: blah-blah-bulls
    clientSecret: blah-blah-bulls
  paypalPlus.client:
    id: blah-blah-bulls
    secret: blah-blah-bulls
    mode: sandbox/live

tenant.pegasus:
  ctp.client:
    projectKey: blah-blah-pegasus
    clientId: blah-blah-pegasus
    clientSecret: blah-blah-pegasus
  paypalPlus.client:
    id: blah-blah-pegasus
    secret: blah-blah-pegasus
    mode: sandbox/live

Environment variables example:

TENANTS=bulls,pegasus
TENANT_bulls_CTP_CLIENT_PROJECTKEY=blah-blah-bulls
TENANT_bulls_CTP_CLIENT_CLIENTID=blah-blah-bulls
TENANT_bulls_CTP_CLIENT_CLIENTSECRET=blah-blah-bulls
TENANT_bulls_PAYPALPLUS_CLIENT_ID=blah-blah-bulls
TENANT_bulls_PAYPALPLUS_CLIENT_SECRET=blah-blah-bulls
TENANT_bulls_PAYPALPLUS_CLIENT_MODE=sandbox/live
TENANT_pegasus_CTP_CLIENT_PROJECTKEY=blah-blah-pegasus
TENANT_pegasus_CTP_CLIENT_CLIENTID=blah-blah-pegasus
TENANT_pegasus_CTP_CLIENT_CLIENTSECRET=blah-blah-pegasus
TENANT_pegasus_PAYPALPLUS_CLIENT_ID=blah-blah-pegasus
TENANT_pegasus_PAYPALPLUS_CLIENT_SECRET=blah-blah-pegasus
TENANT_pegasus_PAYPALPLUS_CLIENT_MODE=sandbox/live
andrii-kovalenko-ct commented 7 years ago

@butenkor @lojzatran @floriansattler what do you think: should we keep tenant name case sensitive or insensitive? i mean, if the customer provides 2 tenant names bulls,pegasus should we also treat them as BULLS,PEGASUS? And respectively the supported URLs will be: http://host.dom/bulls/commercetools/handle/payments/XXX-XXX == http://host.dom/BULS/commercetools/handle/payments/XXX-XXX and http://host.dom/pegasus/paypalplus/notification/ == http://host.dom/PEGASUS/paypalplus/notification/

Note: this is easy to implement from REST controller prospective, since we have dynamic URL handling like this

The question appeared because of Spring default config reading behavior: as i know they skip characters case. As an advantage of this it is easy to supply properties to the application using either environment vars, yml, properties files, json configs and so on.

See more info in Spring Boot Relaxed Binding

My vote is to make tenant name/configs/URLs case insensitive.

What do you think?

lojzatran commented 7 years ago

My vote is also case INsensitive. Case sensitive causes more confusion.

andrii-kovalenko-ct commented 7 years ago

@butenkor @floriansattler @lojzatran
as you remember, in payone-integration we use properties as a set of environment variables, like this:

TENANTS=ONE, TWO
ONE_TENANT_PAYONE_KEY=XXX
ONE_PAYONE_MERCHANT_ID=XXX
ONE_CT_PROJECT_KEY=XXX
ONE_CT_CLIENT_ID=XXX

TWO_TENANT_PAYONE_KEY=XXX
TWO_PAYONE_MERCHANT_ID=XXX
TWO_CT_PROJECT_KEY=XXX
TWO_CT_CLIENT_ID=XXX

Spring framework has much more flexible way to setup the external configuration, including such a feature as JSON parsing, so we could specify the properties hierarchal like this:

SPRING_APPLICATION_JSON='{  
   "tenantConfig":{  
      "tenants":[  
         {  
            "name":"paypalplus-integration-test",
            "ctp":{  
               "projectKey":"paypalplus-integration-test",
               "clientId":"xxx",
               "clientSecret":"xxx"
            },
            "paypalPlus":{  
               "id":"xxx",
               "secret":"xxx",
               "mode":"sandbox"
            }
         },
         {  
            "name":"paypalplus-integration-test",
            "ctp":{  
               "projectKey":"paypalplus-integration-test",
               "clientId":"xxx",
               "clientSecret":"xxx"
            },
            "paypalPlus":{  
               "id":"xxx",
               "secret":"xxx",
               "mode":"sandbox"
            }
         }
      ]
   }
}'

(where SPRING_APPLICATION_JSON is an environment variable)

My question is: should we stick with the old configuration way (e.g. environment variables with prefixes) or we could use the new one, with JSON/YAML format?

There are 2 benefits of the new approach:

butenkor commented 7 years ago

I guess new approach is ok and not only because of Spring. Last approach was just based on previous implementation structure. We need to check if long env values can be easily passed to docker container and travis.