Fix AzureCommentReporter not adding comments to PR

oxsecurity / megalinter

🦙 MegaLinter analyzes 50 languages, 22 formats, 21 tooling formats, excessive copy-pastes, spelling mistakes and security issues in your repository sources with a GitHub Action, other CI tools or locally.

https://ox.security

GNU Affero General Public License v3.0

1.97k stars 238 forks source link

Fix AzureCommentReporter not adding comments to PR #4138

Closed lukelloyd1985 closed 1 month ago

lukelloyd1985 commented 1 month ago

Fixes #4132

Proposed Changes

Use SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI instead of BUILD_REPOSITORY_ID to locate the correct repository referenced in the PR

Readiness Checklist

Author/Contributor

[x] Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
[x] 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

echoix commented 1 month ago

@nvuillam will that new env car be passed to the docker image on azure DevOps without further changes?

@lukelloyd1985 Will existing projects on Azure DevOps stop working if they had to pass the old env var (or it is passed without any user-specific changes)?

nvuillam commented 1 month ago

@nvuillam will that new env car be passed to the docker image on azure DevOps without further changes?

@lukelloyd1985 Will existing projects on Azure DevOps stop working if they had to pass the old env var (or it is passed without any user-specific changes)?

@echoix good catch, that would be indeed a breaking change ^^

@lukelloyd1985 please can you implement the following behavior:

if SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI has a value, then use it
otherwise keep using BUILD_REPOSITORY_ID

Note: I think the default azure pipelines from documentation are ok, because they use --env-file <(env | grep -e SYSTEM_ -e BUILD_ -e TF_ -e AGENT_) \ , so SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI should be available

lukelloyd1985 commented 1 month ago

All (SYSTEM, BUILD etc) variables are passed by the --env-file line in the pipeline as you've noted.

SYSTEM & BUILD variables are already being utilised.

I've already added an if statement to log if the variable happens to be empty (following the logic elsewhere in the script). Should that be sufficient?

It is the PR reporter so needs to reference the correct PR branch which BUILD_REPOSITORY_ID doesn't do (unless the PR and build pipeline happen to be the same repository).

nvuillam commented 1 month ago

@lukelloyd1985 there are many custom pipelines using MegaLinter, probably without --env-file <(env | grep -e SYSTEM_ -e BUILD_ -e TF_ -e AGENT_) but with a given list of variables, like in the previous versions of MegaLinter azure-pipelines default workflow, and I don't want them to crash if they have BUILD_REPOSITORY_ID and not SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI ^^

That's why I'd like your updates to have a "fallback" with just BUILD_REPOSITORY_ID if it's the onlly one available :)

lukelloyd1985 commented 1 month ago

Righto 👌🏻 will action that

lukelloyd1985 commented 1 month ago

How's that?

nvuillam commented 1 month ago

How's that?

Looks good :)

I added a last comment :)