-
Notifications
You must be signed in to change notification settings - Fork 43
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
CGMES export: fix regression on external explicit SV dependencies #2991
CGMES export: fix regression on external explicit SV dependencies #2991
Conversation
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
.addDependentOn(getExportedTPModel().getId()) | ||
.addDependentOn(getExportedSSHModel().getId()); | ||
getExportedSVModel().clearDependencies(); | ||
List<String> tpIds = legacyIdsForSvDependencies.get(CgmesSubset.TOPOLOGY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your solution but I was thinking of something different:
- No properties anymore ;
- We keep information in DependentOn fields ;
- I was thinking about a simple parameter at export that will just say if we export using the extension entirely defined by the user in SV part, meaning that we trust the user. If not, we are allowed to clear the dependencies of SV in extension and to use the other models' dependencies (TP and SSH).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the second option we discussed, but it will require some changes in the code where the issue was raised. I first wanted to provide a fix that did not required changes in the code already written by the users.
Additionally, building the extension for SV model entirely from scratch requires the user to know some information that now is managed by the CGMES export in a transparent way (mainly, the profile URLs, that are different for each version of CGMES exported).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I am already preparing a PR that lets the user give the metadata models as inputs, as you suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, with the option relying on a parameter, we must keep the properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2993 implements your proposal about respecting the info about dependencies present in the extension. It requires that the external user builds the metadata model in the extension. An example of this, following the same coding scheme seen in the software that raised the issue, can be found in the unit test LegacyCommonGridModelExportTest::buildSvDependenciesManaginMetadataModels
.
I would keep (for now) the two ways of solving the problem:
- Accept updated dependencies through the legacy mechanism of properties (so the external users do not have to change their code).
- Let the user manage dependencies through metadata model (with an example provided in our code).
Both PRs can be merged without conflicts. #2993 is based on this one and can be merged after it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Properties management should be removed for next version, here is only to ease transition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Properties management should be removed for next version (2024.2.0).
|
) Signed-off-by: Luma <zamarrenolm@aia.es> (cherry picked from commit 021dc01)
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Some users of the PowSyBl framework were setting explicitly the dependencies for CGMES SV exported file through properties stored in the network.
The recent refactoring of the CGMES metadata models (#2898) introduced a regression in this feature.
What is the new behavior (if this is a feature change)?
The feature for explicitly setting SV dependencies through network properties is kept.
It has been implemented for legacy support. The users of this feature do not need to change their code.
In the future, it is expected that users of CGMES CGM SV export rely directly on the CGMES CGM export that is being developed in #2927.
Does this PR introduce a breaking change or deprecate an API?
Other information: