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

Add repositories info so the generation of docs TIP and overall TIR have the info they need to render the doc. #100

Draft
wants to merge 26 commits into
base: feature/moveLevaDoc
Choose a base branch
from

Conversation

victorpablosceruelo
Copy link
Contributor

@victorpablosceruelo victorpablosceruelo commented Apr 13, 2022

  • Adds fixture to the tests, basically the repositories info, but some more changes are there too.
  • jekins -> jenkins (in jenkins log param). WARN: this forces to upgrade the shared lib too.
  • computes values gitUrl and doInstall for each repository.
  • improves some development utilities.

@victorpablosceruelo victorpablosceruelo changed the title Enables tests previously disabled. Add repositories info so the generation of docs TIP and overall TIR have the info they need to render the doc. Apr 18, 2022
@@ -1639,4 +1647,15 @@ class LeVADocumentService {
junit.getNumberOfTestCases(testData.testResults) - testIssues.count { !it.isUnexecuted }
}

private computeRepositoriesDataNeededInDocs(Map data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, can you change computeRepositoriesDataNeededInDocs by "updateRepositoryData"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

supplier: "mySupplier-A",
version: "myVersion-A",
references: "myReferences-A"
name: repo.name ? repo.name : repo.id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the other class you said repo.name:
String repoName = repo.id as String
repoName = "${projectId}-${repoName}"
Here is different?
And when is possible to don't have the repo.name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs more work.
Seems to be in the original commit you forgot about this information.
We are now determining how to obtain this information.
Thanks for pointing to it.

@@ -23,7 +28,12 @@ class LevaDocTestValidator {
boolean validatePDF(String buildId) {
unzipGeneratedArtifact(buildId)
if (GENERATE_EXPECTED_PDF_FILES) {
copyDocWhenRecording(buildId)
boolean comparisonResult = compareFiles(buildId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all this part as it will add a lot of time to the test, when if this is GENERATE_EXPECTED_PDF_FILES=true, we expect to copy the results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at the whole change, not just two lines.
We are updating the docs, but only if we need to.
Pdfs source code can change while the visual result remains the same.
Please comment again if you want us to remove it.

Copy link
Member

@s2oBCN s2oBCN Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't put this code, this will add a lot of time and not needed

Victor to so2:
Please do not close conversations if you use soft arguments in your last words. Can't answer them.

It is not needed for you because you did not took time debugging and fixing a single document. Why do I have to update all of them when only one is wrong? Just because you do not like the idea of having two more lines of code? It is nonsense.

@@ -49,6 +59,17 @@ class LevaDocTestValidator {
} else {
FileUtils.copyFile(actualDoc(buildId), reportPdfDoc(buildId))
}

if (!filesAreEqual) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the else of the line 59.
Improve the else of the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll do that if previous change is accepted. If not, we'll remove this lines because they are related to the previous ones.

@@ -0,0 +1,49 @@
package org.ods.doc.gen.leva.doc.fixture

class ProjectRepositoryFixture {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file does not scale to more projects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yours neither. This makes it possible to test ordgp project.
If we remove them, tests fail.
It is obvious that we'll have to fix your work in the future, but right now we can live with it.

Copy link
Member

@s2oBCN s2oBCN Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not true. This data does not scale at all

Victor to so2:
Please do not close conversations if you use soft arguments in your last words. Can't answer them.
Yours code in LevaDocDataFixture scales, right?
Example:
targetEnvironmentToken: "D",

Please, take a look to the code we have to live with just because you can tell us ours is bad while we cannot tell you yours can be improved.

@@ -26,6 +26,7 @@ class LevaDocDataFixture {
data.build = buildJobParams(projectFixture)
data.git = buildGitData(projectFixture)
data.openshift = [targetApiUrl:"https://openshift-sample"]
data.repositories = new ProjectRepositoryFixture().load()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were using getModuleData()
If we don't need it anymor,, please clean the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getModuleData is still used to generate component information sent. Please notice that when documents use component information they do not use repositories information.

… github.com:/opendevstack/ods-document-generation-svc into feature/add-component-params-to-overall-tir-and-tip
@victorpablosceruelo victorpablosceruelo marked this pull request as draft April 19, 2022 13:01
@@ -23,7 +28,12 @@ class LevaDocTestValidator {
boolean validatePDF(String buildId) {
unzipGeneratedArtifact(buildId)
if (GENERATE_EXPECTED_PDF_FILES) {
copyDocWhenRecording(buildId)
boolean comparisonResult = compareFiles(buildId)
Copy link
Member

@s2oBCN s2oBCN Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't put this code, this will add a lot of time and not needed

Victor to so2:
Please do not close conversations if you use soft arguments in your last words. Can't answer them.

It is not needed for you because you did not took time debugging and fixing a single document. Why do I have to update all of them when only one is wrong? Just because you do not like the idea of having two more lines of code? It is nonsense.

@@ -26,6 +26,7 @@ class LevaDocDataFixture {
data.build = buildJobParams(projectFixture)
data.git = buildGitData(projectFixture)
data.openshift = [targetApiUrl:"https://openshift-sample"]
data.repositories = new ProjectRepositoryFixture().load()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1- The data in getModuleDatais the same you put here.
2- Shared is sending this data for everytype of doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1-That is not true. While the previous contains only info for a component, new code cotains different information for each code.
2-If you are asking this, you did not expend enough time reading my code.

@@ -0,0 +1,49 @@
package org.ods.doc.gen.leva.doc.fixture

class ProjectRepositoryFixture {
Copy link
Member

@s2oBCN s2oBCN Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not true. This data does not scale at all

Victor to so2:
Please do not close conversations if you use soft arguments in your last words. Can't answer them.
Yours code in LevaDocDataFixture scales, right?
Example:
targetEnvironmentToken: "D",

Please, take a look to the code we have to live with just because you can tell us ours is bad while we cannot tell you yours can be improved.

@metmajer
Copy link
Member

@s2oBCN @victorpablosceruelo I suggest we move the conversation to an internal place now

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.

4 participants