Skip to content
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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

augustocristian
Copy link
Contributor

This PR is related to #58

  • Includes new module with:
    • Most of the model entities
    • Code-functionality of the resource identification
    • Test cases that validate the above functionality
  • Necessary changes in the pipelining code (actions) to make all pass-work

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)

…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
@augustocristian
Copy link
Contributor Author

Good afternoon all!
As suggested by @javiertuya, I've changed the approach to something more incremental:

  1. Resource identification
  2. Grouping+Scheduling
  3. Deployment+Orchtools
    Each stage would include the necessary tests, configuration files and other tool stuff. On this PR, I've cleaned and presented the code to make everything as much clean as possible. I've decided to create a new module and not integrate-rename into the annotations due to two important factors (I'think that Javier make this structure for the same reason).
  4. The tool can be used once or several times to generate the necessary scripting-pipelining code, but this code-methods can be removed when everything is generated (and avoid get them from our repository at each test suite execution)
  5. The annotations conf-files always remain with the test cases.
    The pipeline is reporting problems in sonar, I've been reviewing the problem and from the best of my knowledge is a false positive (is reporting the loggers as circular reference)

Copy link
Contributor

@javiertuya javiertuya left a 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

pom.xml Outdated Show resolved Hide resolved
retorch-orchestration/pom.xml Outdated Show resolved Hide resolved
retorch-orchestration/pom.xml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
retorch-orchestration/pom.xml Show resolved Hide resolved
@augustocristian
Copy link
Contributor Author

@javiertuya Several doubts:

  1. What is the best practice for declaring dependencies in parent-children poms?
    1. Declare the dependencies in the parent pom that are going to be used by several modules: JUnit, Loggers, JSON parsers, and declare those dependencies that are only used in certain modules in its pom.
    1. Declare everything in the parent pom, and use it as required in the different modules.
      My knowledge about building cycles in Maven is not big, but as far as I am concerned, the first approach would make that we don't "download-use" those dependencies not required to build a certain module
  2. The reports that Sonar Cloud must see are only the coverage, no? The test execution record is plotted in the actions.
  3. Now the publishing of the snapshots is broken, during the build of the jars is not detecting the annotations library There's some configuration of the javiertuya/branch-snapshots-action@v1.2.3 action that I am missing? I've tried to use the same flag than the test execution (experienced the same problem in the past) : -am but It didn't work

Thanks a lot for everything!

@javiertuya
Copy link
Contributor

@augustocristian

@javiertuya Several doubts:

  1. What is the best practice for declaring dependencies in parent-children poms?
    1. Declare the dependencies in the parent pom that are going to be used by several modules: JUnit, Loggers, JSON parsers, and declare those dependencies that are only used in certain modules in its pom.
    1. Declare everything in the parent pom, and use it as required in the different modules.

See our template https://github.com/giis-uniovi/samples-giis-template for an example:

  • all project dependencies declared in the dependenciesManagement in the parent pom, This does not import any dependency, it is only declaration
  • each module imports (inherits) the dependencies that needs: only group and artifact is needed.
  • the exception are other modules in the project that need to specify the version
  • for test only dependencies, declare the scope in the parent, it does not need to be declared in the child
  • if the child needs a non test dependency to be used in the tests, specify the scope in the child (this will override the default scope)
  • see https://maven.apache.org/guides/introduction/introduction-to-the-pom.html for more details on poms and inheritance

My knowledge about building cycles in Maven is not big, but as far as I am concerned, the first approach would make that we don't "download-use" those dependencies not required to build a certain module

Parent dependencies in dependenciesManagement are not downloaded, it is only a declaration

  1. The reports that Sonar Cloud must see are only the coverage, no? The test execution record is plotted in the actions.

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).

  1. Now the publishing of the snapshots is broken, during the build of the jars is not detecting the annotations library There's some configuration of the javiertuya/branch-snapshots-action@v1.2.3 action that I am missing? I've tried to use the same flag than the test execution (experienced the same problem in the past) : -am but It didn't work

It is not broken, it is disabled by an if condition. Comment-out this condition, I will check it after the merge

Thanks a lot for everything!

- Moving dependencies to the parent.
- Removing hardcoded dependency to annotations
- Aligning actions with giis-samples
Copy link
Contributor

@javiertuya javiertuya left a 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.

@javiertuya
Copy link
Contributor

@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.

image

@augustocristian
Copy link
Contributor Author

Sorry, I've received the notification in the email:
image
And I thought that you have finished with the review @javiertuya

- 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
Copy link
Contributor

@javiertuya javiertuya left a 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
@augustocristian
Copy link
Contributor Author

augustocristian commented Nov 15, 2024

@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.
However, I’m still unclear if you want this part to be addressed at the end of the migration, or if we should start the packaging process now (e.g., configuring the appropriate plugin to generate a JAR for this module as an artifact).
If you want to be addressed later, I created a couple of hours ago one issue liked to your comment #60 (but not notified anything)
Thanks for everything!

@javiertuya
Copy link
Contributor

@augustocristian It is simple, this is what I wrote:

This could be posponed, in this case, create an issue for it to handle just after this PR is merged (and place the id here)

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).

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.

2 participants