SED-4672 fragments newly added via fragment manager are not registered in automation package yaml#653
Conversation
…ed-via-fragment-manager-are-not-registered-in-automation-package-yaml # Conflicts: # step-ap-ide/src/main/java/step/ap_ide/StepUp.java # step-automation-packages/step-automation-packages-manager/src/main/java/step/automation/packages/AutomationPackageReader.java # step-automation-packages/step-automation-packages-yaml/src/main/java/step/automation/packages/yaml/AutomationPackageYamlFragmentManager.java # step-automation-packages/step-automation-packages-yaml/src/main/java/step/automation/packages/yaml/deserialization/AbstractYamlAutomationPackageFragmentDeserializer.java # step-automation-packages/step-automation-packages-yaml/src/main/java/step/automation/packages/yaml/model/AbstractAutomationPackageFragmentYaml.java # step-automation-packages/step-automation-packages-yaml/src/main/java/step/automation/packages/yaml/model/AutomationPackageFragmentYaml.java # step-automation-packages/step-automation-packages-yaml/src/test/java/step/core/yaml/PatchingContextTest.java # step-core-model/src/main/java/step/core/yaml/PatchableYamlModelBase.java # step-core-model/src/main/java/step/core/yaml/PatchingContext.java
|
Hi @cl-exense I have only unit tested so far, will play around with IDE for more rigorous test before I leave it for review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the automation package fragment management by transitioning from URL-based to Path-based operations and introducing PatchableYamlPrimitive to handle fragment paths dynamically. It also implements functionality to rename and move fragment files on disk when entities are modified. The review feedback highlights several critical cross-platform compatibility issues on Windows, such as path conversion failures, path separator mismatches in glob matching and comparisons, and potential IO exceptions when moving non-existent files, alongside a potential null pointer exception in PatchableYamlPrimitive.
…ed-via-fragment-manager-are-not-registered-in-automation-package-yaml
…ia-fragment-manager-are-not-registered-in-automation-package-yaml' into SED-4672-fragments-newly-added-via-fragment-manager-are-not-registered-in-automation-package-yaml # Conflicts: # step-automation-packages/step-automation-packages-yaml/src/main/java/step/automation/packages/yaml/AutomationPackageYamlFragmentManager.java
cl-exense
left a comment
cl-exense
left a comment
There was a problem hiding this comment.
Generally looks ok, just a few questions and suggestions for improvements .
|
@cmisev We have the additional issue that this branch and the one I worked on (SED-4557) have slightly diverged. Not terribly much, but there will be a few conflicts. I suggest to address the review comments first, then we can see how to consolidate. The 4557 is still ongoing, so if this here can then be considered finished, it makes sense to merge it to 4427 (and then into 4557, fixing conflicts). |
|
Hi @cl-exense thanks for the comments, will adress them so we can quickly move on. I am not too much worried about potential divergence with SED-4557 but rather between SED-4557 and SED-4770, would you mind creating a draft PR for SED-4557 so I can see your current state? |
|
@cl-exense Just saw your draft https://github.com/exense/step/pull/667/changes#diff-3a67b8eed37d9e216d5c4bc4c2e11223a0a0481967d94bf40ca862e4f99d69eb , am not all to worried about divergence for now. |
2 participants
No description provided.