SED-4429 Step AP IDE (Automation Package Local Editor) - #594
Draft
cmisev wants to merge 91 commits into
Draft
Conversation
Contributor
Summary of ChangesHello @cmisev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational components for enhanced IDE integration with automation packages. It introduces a dedicated module and refactors the core mechanism for reading and managing automation package content, specifically plans defined in YAML fragments. The changes enable direct manipulation of these plans, paving the way for future IDE features that can seamlessly interact with and persist changes to automation package definitions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new step-automation-packages-ide module to support mapping YAML fragments to plans, which is a great step towards better IDE integration. The changes include a new collection factory and collection implementation for plans, along with significant refactoring in AutomationPackageReader to support a tree structure of fragments. My review focuses on improving dependency management, fixing a potential bug with a hardcoded limit, and enhancing exception handling for better maintainability.
Comment on lines
+52
to
+57
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-all</artifactId> | ||
| <version>1.9.5</version> | ||
| <scope>test</scope> | ||
| </dependency> |
Contributor
There was a problem hiding this comment.
The mockito-all dependency is used with a very old version 1.9.5 (from 2012). This artifact is deprecated and can cause dependency conflicts as it bundles other libraries. The project already includes mockito-core (lines 47-50), which is the recommended approach. Please remove the mockito-all dependency to avoid potential issues and keep the dependencies clean.
|
|
||
| @Override | ||
| public void remove(Filter filter) { | ||
| find(filter, null, null, null, 100).forEach(fragmentManager::removePlan); |
Contributor
There was a problem hiding this comment.
The remove method uses a hardcoded limit of 100 when finding plans to remove. If more than 100 plans match the filter, only the first 100 will be removed by fragmentManager::removePlan, but super.remove(filter) will remove all matching plans from the in-memory collection. This will lead to an inconsistent state between the file system and the in-memory representation. To fix this, you should fetch all matching plans, for example by passing Integer.MAX_VALUE as the limit.
Suggested change
| find(filter, null, null, null, 100).forEach(fragmentManager::removePlan); | |
| find(filter, null, null, null, Integer.MAX_VALUE).forEach(fragmentManager::removePlan); |
Comment on lines
+115
to
+117
| } catch (URISyntaxException | IOException e) { | ||
| throw new RuntimeException(e); | ||
| } |
Contributor
There was a problem hiding this comment.
Catching URISyntaxException | IOException and re-throwing it as a generic RuntimeException can obscure the original cause of the error and make debugging more difficult. It is better to wrap the original exception in a more specific, custom exception or at least include a descriptive message in the new RuntimeException.
Suggested change
| } catch (URISyntaxException | IOException e) { | |
| throw new RuntimeException(e); | |
| } | |
| } catch (URISyntaxException | IOException e) { | |
| throw new RuntimeException("Failed to write fragment: " + fragment.getFragmentUrl(), e); | |
| } |
…esources-layout' into SED-4429-step-ap-ide
* SED-4663-fix-vulnerable-test-dependency-xmlunit-assertj (#637) SED-4663 Fix vulnerable xmlunit-assertj dependency * SED-4666 Small improvements to startup and shutdown (#638) * SED-000 fixing OS BE local test config * SED-4578 Refactor test classes/resources layout * SED-4678 Rename module * SED-4679 Fix automation-packages-controller/pom.xml (#641) * SED-4673 Improved errors for inexistent fragment references in AP (#642) * SED-4673 Graceful handling of inexistent fragment references in AP * SED-4673 Add another variant * SED-4673 AI PR feedbacks; consistent logger naming and usage * SED-4673 Updated according to discussion * SED-4681 Process automation-package.yaml once, not twice (#643) * SED-4681 Process automation-package.yaml once, not twice * SED-4681 Adjusting test file * SED-4681 Fixing Format --------- Co-authored-by: David Stephan <david.stephan@exense.ch>
…-package-collection-and-accessor
…to SED-4429-step-ap-ide
* SED-4692 Refactor YAML patching logic * SED-4429 bugfixes and other improvements
…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
…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
…d in automation package yaml (#653)
cmisev
changed the title
* SED-4692 Refactor YAML patching logic * SED-4429 bugfixes and other improvements * SED-4672 make fragments list patchable and add glob pattern if not included * SED-4672 address code assist review * SED-4672 remove fragments if completely empty * SED-4672 fix existing reference determination * SED-4672 fix cloning * SED-4672 new default for plan creation * SED-4672 new unit tests for keyword adding and removal * SED-4557 WIP * SED-4672 refactor fragment manager * SED-4672 separate mapping directions, add node and jmeter mapping * SED-4672 review comments * SED-4672 review comments, make collectionfactory generic * SED-4672 review comments * SED-4770 skip uploading of local files * SED-4557 Remove .gitignore * SED-4770 only output modified fields * SED-4672 review comments * SED-4770 paths relative to AP for General Script Keywords * SED-4770 common fields copy * SED-4770 dynamic value equals implementation * SED-4557 WIP * SED-4557 introducing AP descriptor * SED-4770 review comments * SED-4770 PR comments round 1 - low hanging fruit * SED-4557 Fixing class cast exception on execution table * SED-4770 Update/Simplify mapping annotations * SED-4557 Fixing AP IDE when working on windows * SED-4770 Refactor collection name methods * SED-4770 Remove redundant fragments tracking * SED-4770 Final touches * SED-4557 PR feedbacks --------- Co-authored-by: Cyril Misev <cyril.misev@exense.ch> Co-authored-by: cmisev <72265111+cmisev@users.noreply.github.com> Co-authored-by: Jérôme Comte <jerome.comte@exense.ch>
…#677) * SED-4688 unclear-error-when-an-ap-contains-no-plan (#658) * SED-4688 Unclear error when an AP contains no plan * SED-4688 PR feedbacks * SED-4709 Improve resilience of Node.js agent when communicating with the controller (#665) * SED-4709 Improve resilience of Node.js agent when communicating with the controller * SED-4709 PR feedbacks * SED-4798 Interrupted execution still show running nodes (#664) * SED-4798 Interrupted execution still show running nodes * SED-4798 thread safe implementation * SED-4798 PR feedback * SED-4800 CLI Automation Package deployment fails with timeout when waiting for active executions to complete * SED-4800 CLI Automation Package deployment fails with timeout when waiting for active executions to complete * SED-4800 PR feedbacks * SED-4800 PR feedbacks * SED-4800 acceptance tests feedback * SED-4785-screentemplates-with-empty-attributes-break-view (#663) SED-4786 adding junit tests to cover records in collections * SED-4764 global-schedules-can-be-toggled-from-other-projects (#656) SED-4764 Global Schedules can be toggled from other projects Co-authored-by: Tim Rasim <neogucky@gmail.com> * SED-4713 isolated-automation-package-repository-canonical-plan-names-are-not-canonical (#657) * SED-4713 IsolatedAutomationPackageRepository canonical plan names are not canonical * SED-4713 PR feedbacks * SED-4758 Implement Label Value Cardinality Safeguards for Custom Metrics Ingestion (#660) * SED-4758 Implement Label Value Cardinality Safeguards for Custom Metrics Ingestion * SED-4786 Introduce execution notices * SED-4786 PR feedbacks * SED-4786 PR feedbacks * SED-4790 more vulnerability fixes (#668) * SED-4790 Bump dependencies * SED-4790 Update transitive exported dependencies * SED-4790 Finalizing dependencies * SED-000 bumping Step version to 3.30.1 * SED-4781-support-text-preview-for-non-streaming-attachments (#670) SED-4781 Support text preview for resources * SED-4789 Implement live reporting in the Node.js agent (#661) * SED-4789 Implement live reporting in the Node.js agent * SED-4789 PR feedbacks * SED-4789 PR feedback: splitting live-reporting.js * SED-4809 CLI changes of SED-4800 are not backward compatible (#669) * SED-4809 CLI changes of SED-4800 are not backward compatible * SED-4809 PR update * SED-4746 grid monitoring dashboard (#662) * SED-4746-update grid monitoring dashboard * SED-4746-update colors for all grid token statuses * Update step-controller-plugins/step-controller-plugins-timeseries/src/main/java/step/plugins/timeseries/dashboards/DashboardsGenerator.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * SED-4746-update tokens labels --------- Co-authored-by: lucian-baciu <lucian.baciu@wedosoft.co> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: David Stephan <David.Stephan@exense.ch> * SED-4810-release-step-30-1 (#671) SED-4810 Release Step 30.1 Co-authored-by: Christoph Langguth <christoph.langguth@exense.ch> * SED-4809 resolve openAPI name conflict for AsyncTaskStatus * SED-4659 JMeter Memory leak in JMeterLocalHandler.updateClasspathSyst… (#673) * SED-4659 JMeter Memory leak in JMeterLocalHandler.updateClasspathSystemProperty * SED-4659 PR feedback * SED-4005 added nextExecutionTimestamp on schedulers (#672) * SED-4005-added nextExecutionTimestamp on schedulers * SED-4005-refactor + cleanup * SED-4005-change schedulerTaskEnricher * SED-4005-pr changes --------- Co-authored-by: lucian-baciu <lucian.baciu@wedosoft.co> Co-authored-by: Tim Rasim <neogucky@gmail.com> * SED-4846 fix-tenant-header-loss-in-nested-remote-clients (#675) * SED-4846 Fix tenant header loss in nested remote clients * SED-4846 PR feedbacks * SED-4846 PR feedbacks * SED-4680 bumping grid to master --------- Co-authored-by: David Stephan <David.Stephan@exense.ch> Co-authored-by: Jérôme Comte <jerome.comte@exense.ch> Co-authored-by: Tim Rasim <neogucky@gmail.com> Co-authored-by: Lucian Baciu <103116491+lucianbaciu@users.noreply.github.com> Co-authored-by: lucian-baciu <lucian.baciu@wedosoft.co> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.