Skip to content

Conversation

@natthan-pigoux
Copy link

Verify the CS config using diracx model validation before committing. If validation fails a warning is diplayed.

closes #8302

@natthan-pigoux
Copy link
Author

I'm not completely sure the verification was expected to be in Modifcator.
Also not sure where to write the diracx related code.

@natthan-pigoux natthan-pigoux force-pushed the feat_diracx_verification_on_cs_commit branch from 3261199 to 7cd1dd8 Compare October 23, 2025 13:25
@fstagni
Copy link
Contributor

fstagni commented Oct 23, 2025

I think your understanding is correct, the check should be at modification time.

I would not create a new module, you can add your new function inside Modificator.py or inside src/DIRAC/FrameworkSystem/Utilities/diracx.py

@natthan-pigoux
Copy link
Author

Thanks @fstagni for the clarification.

@natthan-pigoux
Copy link
Author

One possible issue I see in what I've done is that, if my understanding is correct, in Modificatior, cfgData corresponds to the entire CS config. If there is many validation errors, each time there is a commit all of these are displayed and not only the ones related to the changes.

Is that ok like this? or should I verify only the current changes?

@fstagni
Copy link
Contributor

fstagni commented Oct 24, 2025

It's probably OK. I am also wondering if instead of simply displaying a warning, the commit should simply be refused.

@natthan-pigoux
Copy link
Author

It's probably OK. I am also wondering if instead of simply displaying a warning, the commit should simply be refused.

In the issue it is precised that:

Not a failure, because we do not want to prevent the commit from happening

yield client


def diracxVerifyConfig(cfgData):
Copy link
Member

Choose a reason for hiding this comment

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

This won't work for converting the CS, see https://diracx.diracgrid.org/en/latest/admin/how-to/install/convert-cs/ for the procedure.

Running it in a subproces with a timeout is probably a good idea.

Copy link
Author

Choose a reason for hiding this comment

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

Ah indeed. Then I will call the diracx cli to validate the config.

@chrisburr
Copy link
Member

It's probably OK. I am also wondering if instead of simply displaying a warning, the commit should simply be refused.

Here me and @chaen disagree but I also think it should be refused. Maybe it would be good to gate it behind an option in the CS service.

@chrisburr chrisburr marked this pull request as draft October 27, 2025 10:17
@natthan-pigoux natthan-pigoux force-pushed the feat_diracx_verification_on_cs_commit branch 2 times, most recently from 014be21 to 90c38d1 Compare November 3, 2025 16:26
@natthan-pigoux natthan-pigoux marked this pull request as ready for review November 4, 2025 16:10
@natthan-pigoux natthan-pigoux marked this pull request as draft November 4, 2025 16:28
@natthan-pigoux natthan-pigoux marked this pull request as ready for review November 5, 2025 16:00
@fstagni fstagni force-pushed the feat_diracx_verification_on_cs_commit branch from 1b3a58e to 8036c6e Compare November 17, 2025 13:34
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.

A commit in the CS should verify the diracx verification

3 participants