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 ability to set multiple report paths in SonarQube actions #674

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

chudilka1
Copy link
Contributor

No description provided.

@chudilka1 chudilka1 requested a review from a team as a code owner October 16, 2024 16:44
id: sonarqube_report_paths
shell: bash
run: |
sonarqube_coverage_report_paths=$(find -type f -name '${{ inputs.typescript-lcov-report-path }}' -printf "%p,")
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this line do?
What is expected fromat for passing multiple paths in the input? Is it a string with newlines as delimiter?

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice if you can document what is expected format of the input parameter in the input description

Copy link
Contributor Author

@chudilka1 chudilka1 Oct 17, 2024

Choose a reason for hiding this comment

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

It recursively looks for the file names specified in action input typescript-lcov-report-path (that corresponds to the report name) and assigns those values to this variable, used later by Sonar. It is tested here, you may find paths delimited by coma (%p, where p is a path/value found)
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the input variable names, so users may now specify only report names instead of paths (the latter will be found automatically). If users specify paths, the action will fail, as it will look for file names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining, so it assumes that all packages, modules in the project produce a report with the same file name? right?

It probably helps for some use cases, but for the other use cases the user of the workflow is not able to specify explicitly which exact report paths should be used. Can this be a problem for mono repos with variety of different frameworks and tools for generating such reports?

Copy link
Contributor Author

@chudilka1 chudilka1 Oct 17, 2024

Choose a reason for hiding this comment

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

Thanks for asking.

modules in the project produce a report with the same file name? right?

Yes, that is how it should be and is in our repos.

The other use cases, the user of the workflow is not able to specify explicitly which exact report paths should be used?

  1. I do not know a use case that would not be solved with the logic proposed in the action. Give me an example if you know, please.
  2. I've attached a link with the example for monorepo above. Have you had a chance to take a look?
  3. There is no need to name files differently. Moreover, it may and will be our standard (though users may still use any custom names for reports and fetch those correspondingly). ATM, this action is report-paths-agnostic; it is only name-sensitive. Why?
  • reports, when generated, are and may be uploaded by any path:
    - name: Upload Golangci Lint artifacts
      if: always()
      uses: actions/upload-artifact@v4.4.3
      with:
        name: ${{ matrix.projects }}_golangci-lint-report
        path: |
          ./${{ matrix.projects }}/golangci-lint-report.xml
  • when those are downloaded, the reports will be located under different dirrectiories/artifacts and will always be fetched by the same reg-exp-name.
  • the reports are merged by Sonar

Copy link
Contributor Author

@chudilka1 chudilka1 Oct 17, 2024

Choose a reason for hiding this comment

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

As an option we may search starting with * (-name '*$...' in $(find -type f -name '*${{ inputs.test-report-name }}' -printf "%p,"), in that case files may be prefixed. However, I'd not recommend to go for this option, as such artifacts as race_coverage.txt may be fetched as well, however, it is not an appropriate artifact (at may cause timeout exception because of its size that may take GBs)

Copy link
Contributor

@scheibinger scheibinger left a comment

Choose a reason for hiding this comment

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

I left an inline comment

actions/ci-sonarqube-go/action.yml Outdated Show resolved Hide resolved
actions/ci-sonarqube-ts/action.yml Outdated Show resolved Hide resolved
id: sonarqube_report_paths
shell: bash
run: |
sonarqube_coverage_report_paths=$(find -type f -name '${{ inputs.typescript-lcov-report-path }}' -printf "%p,")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining, so it assumes that all packages, modules in the project produce a report with the same file name? right?

It probably helps for some use cases, but for the other use cases the user of the workflow is not able to specify explicitly which exact report paths should be used. Can this be a problem for mono repos with variety of different frameworks and tools for generating such reports?

@chudilka1 chudilka1 force-pushed the fix-sonarqube-action-2 branch 3 times, most recently from f20d314 to c580fb6 Compare October 17, 2024 17:52
@chudilka1 chudilka1 marked this pull request as draft October 17, 2024 18:16
scheibinger
scheibinger previously approved these changes Oct 18, 2024
Copy link
Contributor

@scheibinger scheibinger left a comment

Choose a reason for hiding this comment

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

Thanks for the answers on my questions, now it is all clear! LGTM

@chudilka1 chudilka1 marked this pull request as ready for review October 18, 2024 18:12
Copy link
Contributor Author

@chudilka1 chudilka1 left a comment

Choose a reason for hiding this comment

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

@scheibinger
I have added here the validation of conditions: when one of the workflow names is not specified, trigger the other action that downloads artifacts from the same workflow. When the workflow name that generates a report is not specified, it always means that the action is used by the same workflow that produces reports. How it works may be found in two different repos, that use different inputs:

  1. chainlink-common - different workflows generate reports and are fetched from different workflows
  2. capabilities (monorepo) - fetches different reports from different places, produced by the same workflow, where this action is specified.

@chudilka1 chudilka1 merged commit 5f4a9c9 into main Oct 21, 2024
11 checks passed
@chudilka1 chudilka1 deleted the fix-sonarqube-action-2 branch October 21, 2024 08:51
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