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://megalinter.io
GNU Affero General Public License v3.0
1.8k stars 215 forks source link

enhancement: Gitleaks schema improvements #3675

Closed TommyE123 closed 1 week ago

TommyE123 commented 1 week ago

GitLeaks Add missing schema properties

Fixes #3064

Context:

Three schema references were missing for Gitleaks which causes YAML_V8R to fail. • REPOSITORY_GITLEAKS_PR_COMMITS_SCANREPOSITORY_GITLEAKS_PR_SOURCE_SHAREPOSITORY_GITLEAKS_PR_TARGET_SHA

Testing:

Shows they were missing from the schema

image image

If there are any issues or concerns, please let me know, and I will address them promptly. Thank you for reviewing this PR and I look forward to your feedback. Tom

echoix commented 1 week ago

Were they added manually? Usually they are generated by a script, build.sh --doc. If this doesn't regenerate correctly, it's another issue.

TommyE123 commented 1 week ago

Yes I added them manually.

I was just going off what @nvuillam said in the linked issue that was raised a while back!

echoix commented 1 week ago

The worse is that at some point when the command will be rerun it will get replaced by the actual contents from the descriptor.

TommyE123 commented 1 week ago

Apologies @echoix,

I should have checked in more detail and wasn’t aware the schema was created off the back of the descriptors as well. I’d like to try and do this properly if that’s ok?

Tom

echoix commented 1 week ago

You can run in your branch to see if the contents are different. Maybe it's the same? Maybe the full doc rebuild is done near releases, were these fields new?

TommyE123 commented 1 week ago

Hi @echoix,

These variables are from a previous release and don't get added via the build.sh --doc command, which is why they're missing from the JSON schema. After looking at the .automation/build.py code, it seems like there's currently no support for automatically adding these types of variables to the schema.

While I considered trying to add that functionality, it would require careful planning and testing to ensure it properly handles different variable types and doesn't cause any unintended issues or conflicts with existing variables.

For now, I've reordered these missing variables to match the order that the build.sh --doc command would place them in. This should work until we can come up with a more permanent solution.

Does this approach sound reasonable? I'm open to any other suggestions you might have for dealing with these missed variables and keeping the schema in sync going forward.

Tom

echoix commented 1 week ago

Yes good

echoix commented 1 week ago

Hi @echoix,

These variables are from a previous release and don't get added via the build.sh --doc command, which is why they're missing from the JSON schema. After looking at the .automation/build.py code, it seems like there's currently no support for automatically adding these types of variables to the schema.

While I considered trying to add that functionality, it would require careful planning and testing to ensure it properly handles different variable types and doesn't cause any unintended issues or conflicts with existing variables.

For now, I've reordered these missing variables to match the order that the build.sh --doc command would place them in. This should work until we can come up with a more permanent solution.

Does this approach sound reasonable? I'm open to any other suggestions you might have for dealing with these missed variables and keeping the schema in sync going forward.

Tom

It would be something very useful to have implemented in the docs generation, jsonschema docs generation or even normal build, as updating them manually is error prone (ie: forget to do it), as you found out recently.

I'm grateful for your work, but I'm blocked with the current CI that fails and can't fix them all at once. Nicolas would be able to force the merge of these PRs, but I can't just by myself.

echoix commented 1 week ago

I’ve been merging your PRs one by one, resolving the conflicts on the changelog as we waited too long to merge. Again, thanks for your great work!

nvuillam commented 1 week ago

Indeed, thanks @TommyE123 for your great latest contributions, and thanks @echoix for all your validation & guidance work ❤️