Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Persist assert_integrity feature #1032

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

daogrady
Copy link

@daogrady daogrady commented Feb 18, 2025

Fixes #1020

@daogrady daogrady changed the title Persist assert_integrity feature feat:Persist assert_integrity feature Feb 19, 2025
@daogrady daogrady changed the title feat:Persist assert_integrity feature feat: Persist assert_integrity feature Feb 19, 2025
if (assertIntegrity) {
packageJson.cds ??= {}
packageJson.cds.features ??= {}
packageJson.cds.features.assert_integrity = assertIntegrity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could already prepare for other supported feature flags in a generic way, because once another feature flag will be added, this should be done anyway. Something like

Copy link
Author

@daogrady daogrady Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually started with a generic implementation that would take an arbitrary path into the cds.env, prepare all parent objects in package.json based on the type of what cds.env dictated and finally copied over all appropriate values. And before I knew it, I had a 200 loc function, that somewhat paled in comparison to this 5 line solution for the problem at hand. 😃
I therefore suggest to keep it simple as long as we are dealing with only one value and generalise it later, if we need to carry over more settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. However, I am not really happy with just fixing the passthrough for this one feature. I can imagine other feature flags which are important for a proper build, such as: naming modes (quoted or plain names?), standard database function mappings, …

@stewsk @johannes-vogel @chgeo we have the issue that build relevant options are not per default copied over to the postgres deployer app (found in gen/pg) do you have some idea how to properly copy over build relevant options (which are those?) to the deployer app? If we don't do that, an mbt build && cf deploy will lose all SQL relevant features defined in the app which e.g. has been the case for cds.features.assert_integrity in #1020

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that there's a dedicated section for deployment relevant things. They are spread over different sections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it then in your opinion be sufficient to have a kind of allow-list approach, where we selectively copy over known build relevant options?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if an allow list makes sense if we do not know the options. How is that solved for the hana deployer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

during build for HANA the actual db artifacts are already created with all features found in the cds env. Upon a deployment the hdi artifacts already have the desired features baked in. In contrast to that, for postgres, only a inferred CSN is generated during build. Which does not contain any DB specific transformations, hence lacking the DB features set in the cds env. Upon deployment, the gen/pg/csn.json is transformed to the actual SQL schema. If the deployment is initiated in the gen/pg/ folder, the root app cds env is ignored in favor of the the cds env of the deployer app (gen/pg/package.json) which is empty, invalidating all features set in the apps root.

@daogrady daogrady marked this pull request as ready for review February 19, 2025 11:15
@daogrady daogrady requested a review from joergmann February 19, 2025 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table Constraints not generated for PostgreSQL When Using "cds build --production"
3 participants