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

Fix: PMD sarif report preview unavailable. #4571

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

Conversation

xezzon
Copy link

@xezzon xezzon commented Jan 23, 2025

According to the GitHub documentation, the conversion of absolute URIs to relative URIs for the source root when uploading a SARIF report should not include the file:// scheme.

Fixes #4522

Proposed Changes

  1. Replace file:///github/workspace to empty string for SARIF report before replacing /github/workspace.

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

According to the [GitHub documentation](https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#specifying-the-location-for-source-files), the conversion of absolute URIs to relative URIs for the source root when uploading a SARIF report should not include the `file://` scheme.
@echoix
Copy link
Collaborator

echoix commented Jan 23, 2025

Remember that Megalinter is running in a docker image, and the workspace is mounted into it, so it isn't running directly from the GitHub runner

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

Plz check comments

@@ -96,6 +96,7 @@ def produce_report(self):
os.remove(linter.sarif_output_file)
result_json = json.dumps(sarif_obj, sort_keys=True, indent=4)
# Remove workspace prefix from file names
result_json = result_json.replace("file:///github/workspace", "")
Copy link
Member

Choose a reason for hiding this comment

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

Won't you need replaceAll ?

Copy link
Member

Choose a reason for hiding this comment

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

@xezzon ? :)

Copy link
Author

Choose a reason for hiding this comment

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

What does replaceAll mean? Putting file:///github/workspace/d' into LIST_OF_REPLACEMENTS`?

@xezzon
Copy link
Author

xezzon commented Jan 27, 2025

Remember that Megalinter is running in a docker image, and the workspace is mounted into it, so it isn't running directly from the GitHub runner

Removing the file:// scheme in SARIF is possible, even when running in a Docker container.

I can't think of any other way to fix #4522 without incompatible changes.

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.

PMD sarif report preview unavailable
3 participants