-
Notifications
You must be signed in to change notification settings - Fork 1
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
Orchestration module: Migration of the Resource Identification process code and tests #59
base: main
Are you sure you want to change the base?
Orchestration module: Migration of the Resource Identification process code and tests #59
Conversation
…est cases - Includes new module with: - Most of the model entities - Code-functionality of the resource identification - Test cases that validate the above functionality - Necesary changes in the pipelining code (actions) to make all pass-work
Good afternoon all!
|
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.
As a first stage I've reviewed the structure (to allow renamings before reviewing the source code) and general configuration files (poms, workflows, etc.).
After finish the review we can handle the sonar issue that seems to be a false positive.
Remember, DO NOT FORCE PUSH commits that have already been reviewed.
@ClaudiodelaRiva FYI
retorch-orchestration/src/main/java/giis/retorch/orchestration/model/AccessModeEntity.java
Outdated
Show resolved
Hide resolved
.../src/main/java/giis/retorch/orchestration/resourceidentification/RetorchClassifierUtils.java
Outdated
Show resolved
Hide resolved
retorch-orchestration/retorchfiles/configurations/retorchCI.properties
Outdated
Show resolved
Hide resolved
retorch-orchestration/retorchfiles/configurations/ClassifierUnitTestsSystemResources.json
Show resolved
Hide resolved
...chestration/src/test/java/giis/retorch/orchestration/unitary/components/ClassifierTests.java
Outdated
Show resolved
Hide resolved
- Workflows renamed, tuned to execute the tests and get the report - Fixed Java versions to 16 - Moved log imp to the parent pom - Renamed Entities - Simplified packages - Renamed several classes
@javiertuya Several doubts:
Thanks a lot for everything! |
See our template https://github.com/giis-uniovi/samples-giis-template for an example:
Parent dependencies in
Not only, but this is the most important. The tests and status can be also shown under section Meassures->Coverage->Tests (If I remember well, previous versions of SonarQube displayed a summary in the dashboard, now they are more hidden).
It is not broken, it is disabled by an
|
- Moving dependencies to the parent. - Removing hardcoded dependency to annotations - Aligning actions with giis-samples
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 didn't started reviewing the code as there many unresolved comments. Please, read the comments and address them, not less, not more.
@augustocristian Do not make changes after you request a review and until I request changes because in the next review I will be unable to focus on the changes since last review. For instance, below there is a commit introduced just before I asked for changes, This implies that these changes are hidden when I want to see only changes since last review. |
Sorry, I've received the notification in the email: |
- Undo the accidental package renaming - Moving utils to src.test.orchestration - Change version of java in the last site to 16 (sonar)
- mikepenz/action-junit-report undo to the previous version (v5) that was accidentally changed
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.
@augustocristian @ClaudiodelaRiva Still unresolved comments (ignored by second time)
- Java downgrade to Java 8 version
- Downgrade logback-classic to suport Java 8
- Downgrade Mockito to support Java 8
- Downgrade Mockito to 4.11.0 support Java 8
@javiertuya I don’t want to close the last comment until I’m sure I fully understand what you mean. From the first moment, I completely understood what you wanted: this module will be a Java tool that takes certain parameters (e.g., the package name) and generates the necessary scripting code, without requiring tasks like importing dependencies into the POM or creating a Java class. |
@augustocristian It is simple, this is what I wrote:
You created the issue, ok, we can talk later about this in the issue (NOT IN THIS PR). But in this case I requested to place the id in the comment (shown in bold). This is the way to create follow-up issues, create the issue, refer the issue in the comment and close the issue (because the comment directs to where it will be addressed). |
This PR is related to #58
The PR is still failing reporting some circular dependencies related to the logs that doesn't exist, the only dependencies are between the packages Resource Entity and RetorchUtils (but not viceversa, circular)