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

Merged
merged 28 commits into from
Dec 12, 2024

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/[email protected] 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/[email protected] 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!

@augustocristian
Copy link
Contributor Author

augustocristian commented Dec 3, 2024

@javiertuya I realized today, with Claudio's, that all my comments were unpublished because I unintentionally started a review a couple of weeks ago. I had been waiting for your response, thinking you were busy.
#59 (comment)
Sorry for all

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.

Now including the review of the contents. Just a few comments regarding the description of some classes.

Additionally, add a description of this module in the README.

- Goal of all test classes specified clearly and as short as possible
- Some typos detected in the comments (resource with lowercase) solved
- Included in the README.md a short description of the new module.
README.md Outdated Show resolved Hide resolved
@augustocristian
Copy link
Contributor Author

@javiertuya I think we should also update the ACKs of the repository to include EQUAVEL. But I've got several doubts about use the ACK of llm-rp (It's the official ones, asked-checked with Claudio), or the "old ones"

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
- Reviewed english and expression
- Changed how we refer to the module (tool<>generator)
- Added how currently the scripting code is generated
README.md Show resolved Hide resolved
@augustocristian
Copy link
Contributor Author

@javiertuya, maybe would be a good idea start removing from the old repository all that is migrated and made a "highly coupled" system importing this orchestration module in GitLab, testing that everything that we change not "breaks" the rest of the implementation?

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 still can't execute the jenkinsfile generation

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Clarifying the utility of jenkinsFilePath parameter
Adapting the package name
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.

Go ahead with the next module. This should be the module that generates the jenkinsfile

@javiertuya javiertuya merged commit 7c21eaf into main Dec 12, 2024
7 checks passed
@javiertuya javiertuya deleted the ft_migrateEntitiesAndResourceIdentification branch December 12, 2024 09:58
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