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

Introduce the GitHub Advanced Security workflow #13828

Merged
merged 2 commits into from
Jul 31, 2021

Conversation

timvandermeij
Copy link
Contributor

Supersedes #13737.

GeekMasher and others added 2 commits July 31, 2021 13:32
This can help to find security problems sooner.
This makes it consistent with the GitHub Advanced Security file and,
more importantly, ensures that all steps have a proper name for better
visibility.
@timvandermeij timvandermeij merged commit 3ec1bac into mozilla:master Jul 31, 2021
@timvandermeij timvandermeij deleted the github-advanced-security branch July 31, 2021 11:44
@timvandermeij
Copy link
Contributor Author

Trivial since this is a minor fixup on the original PR, the workflows all pass and this forms the basis of more workflows.

@Snuffleupagus
Copy link
Collaborator

Is the intention now to basically "replace" the LGTM integration, by manually enabling the same rule-set in this new workflow?

The reason for my question is that it feels a bit unfortunate to have two separate systems flagging (mostly) the same things, since we'll have deal with alert suppression twice now (which could be annoying and lead to inconsistencies).


Also, should we perhaps go through the list in https://github.com/mozilla/pdf.js/security/code-scanning to decide/agree upon how to classify the listed alerts?
I suppose that I could try to put together a list with a suggested resolution for every alert, and then we can discuss them?

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Jul 31, 2021

Is the intention now to basically "replace" the LGTM integration, by manually enabling the same rule-set in this new workflow?

For now the idea is that GitHub Advanced Security takes care of the security checks and LGTM takes care of the code quality checks. It might indeed be a good idea to replace LGTM later on and change GitHub Advanced Security to also enable the code quality suite, but since LGTM was just integrated it seemed best to defer that to a follow-up and to first get some experience with both tools; basically #13737 (comment) which I agree with.

I'll file a follow-up issue to, now that the tooling is in place, take a look at the output and decide how to proceed. I'd be perfectly fine with replacing LGTM now and indeed classifying the current list that GitHub Advanced Security shows, especially since the latter is much easier to control given that alerts can be dismissed from the interface instead of needing to have comments in-code (which I find less ideal).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants