CXCDS-14100: move SCI configuration from .env to schematics-config #19952
Conversation
spartacus
|
||||||||||||||||||||||||||||||
| Project |
spartacus
|
| Branch Review |
bugfix/CXCDS-14100
|
| Run status |
|
| Run duration | 12m 18s |
| Commit |
|
| Committer | johannathalmann-SAP |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
3
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
125
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
| "sciEnabled": { | ||
| "type": "boolean", | ||
| "description": "CDS/ISS is integrated into the SAP Cloud Identity Service (SCI). The downstream services use different domains and URL formats. To enable SCI, enable this parameter and make sure to use the proper profile tag urls (see documentation).", | ||
| "default": false, |
There was a problem hiding this comment.
[Business question]
I understand we don't want to enable SCI in existing applications, but rather let customers enable it by themselves
But do we really want to disable by default SCI in fresh new applications created by customers? My gut feeling is we'd like new apps to use newest features OOTB. But you know better the business context here, so I'm happy to learn your poiunt of view.
There was a problem hiding this comment.
SCI support for ISS will be activated a little bit later - we’re still in extensive test phase for that (that’s also why we need to be able to activate it already for our test installations)
But you are right, after SCI is fully integrated into ISS, we could think about making it the default for new setups.
There was a problem hiding this comment.
Thank you. Now I get better the business context.
Do I understand correctly that eventually when we like customers to enable the SCI feature, THEY will have to:
- manually change the endpoints configuration
config.cds.endpointsin theirCdsFeatureModulein their apps? - they can keep the config
config.cds.profileTag.sciEnabledfalse in theirCdsFeatureModulein their apps, becasue it is (and will be?) a dead config
btw. as of today new apps crated with schematics will put the config config.cds.profileTag.sciEnabled explicitly in customers' apps' CdsFeatureModule and [set it there to false](https://github.com/SAP/spartacus/pull/19952/files#diff-924178d81edea4b6d2638cbf2f6783c4a120785a01d00a833ff3dd21c53882e9R110). But it seems to be a dead config (as noted in another comment https://github.com//pull/19952/files#r1940901467) - which I believe can be misleading for customers. Changing this config will have no effect for them, right? So I still struggle to understand why we want to expose this config. Perhaps I'm missing something obvious important - then please help me understand.
There was a problem hiding this comment.
Customers who enable SCI, during their setup, will have to:
- set the sciEnabled = true
- set the correct profiletag urls
Which strategy endpoint to use, is then decided on the value of sciEnabled in the cds-schematics-config.ts (line 74-76)
Also config.cds.profileTag.sciEnabled will be same value as in the setup (line 110)
Or is your question regarding changing these things MANUALLY in CdsFeatureModule only refering to existing setups/customers?
There was a problem hiding this comment.
Thank you for your answer.
So I understand that during initial installation of spartacus, the shape of the scaffolded CdsFeatureModule in customer's app will look differently depending on the value of sciEnabled in the cds-schematics-config.ts (line 74-76).
And I understand that if customer already created an application with CDS in the past, then the only way enable SCI in the future (if they want it in the future), for them will be they'll manually change the content of their CdsFeatureModule -> then we should put instructions in our official docs for them so they know what changes to peform manually there.
Still, what I dont understand is: why do we keep Spartacus config property config.cds.profileTag.sciEnabled. Perhaps we have some future plans for it?
Currently, as far as I can see in the code, it's not used anywhere. It's just defined.
Perhaps I'm missing something here, then please correct me.
If this config is not used anywhere, my gut feeling is we should remove it and re-introduce only possibly in the future, if in the future we add some logic in Spartacus that would use this config.
There was a problem hiding this comment.
Hello, both assumptions for initial installation and already created application are correct. We already gave our input to our documentation team to have that properly written down.
I've deleted the config.cds.profileTag.sciEnabled property.
| @@ -97,6 +107,7 @@ function buildCdsConfig( | |||
| 'PROFILE_TAG_CONFIG_URL_PLACEHOLDER' | |||
| }', | |||
| allowInsecureCookies: true, | |||
| sciEnabled: ${options.sciEnabled} | |||
There was a problem hiding this comment.
[QUESTION]
I can see here we set the property of the Spartacus global config config.cds.profileTag.sciEnabled based on the argument option --sci-enabled of the Installation Schematics, during the installation.
- But I'm having problem to understand what do we need this Spartacus config option for (
config.cds.profileTag.sciEnabled)?
I have problem to find any usages of it in runtime. Below is a screenshot from VSCode on the current branch:
(See:
)- By the way, I can see we've removed an analogical Spartacus config property in this PR in 1 level above
config.cds.sciEnabled(without.profileTag.in beteween). So it seems we had previously the config propertysciEnableddefined in interfaces on both levels:
config.cds.sciEnabled- which is removed in this PRconfig.cds.profileTag.sciEnabled- which remained in this PR
Perhaps we wanted to remove also theprofileTag-nested one?
There was a problem hiding this comment.
We want to have it consistent with our profileTag implementation - https://github.tools.sap/cx-commerce/cds-argonauts-profiletag/blob/master/src/config/config.ts
The value of "sciEnabled" should not be defined in .env_cdmrc file, as that's only available for developers. Instead, it needs to be configured when setting up cds from libs.