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

Create codeql-analysis.yml #2644

Merged
merged 6 commits into from
Dec 31, 2022
Merged

Create codeql-analysis.yml #2644

merged 6 commits into from
Dec 31, 2022

Conversation

Flyingmana
Copy link
Contributor

@Flyingmana Flyingmana commented Oct 3, 2022

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Replace LGTM.com as it's going to shutdown in Dec. 2022 #2642

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sreichel
Copy link
Contributor

Seems LGTM is down ... last check already runs for a hour. (https://github.com/OpenMage/magento-lts/runs/9753202323)

@Flyingmana can you please disable?

@addison74
Copy link
Contributor

Can we move forward with this issue? LGTM.com is already shut down.

@fballiano
Copy link
Contributor

it seems this PR is already complete

@sreichel
Copy link
Contributor

it seems this PR is already complete

But ... 92 new alerts including 87 high severity security vulnerabilities. Should we fix that first?

@fballiano
Copy link
Contributor

where do you see those errors?

@sreichel
Copy link
Contributor

@fballiano
Copy link
Contributor

uff, prototype and tinymce = 99% impossible to solve

@Flyingmana Flyingmana marked this pull request as ready for review December 30, 2022 18:43
@Flyingmana
Copy link
Contributor Author

we dont need to solve this "new errors", maybe put them into an ignore or mark it as allowed to fail

@sreichel
Copy link
Contributor

There seems to be a baseline option ... needs some test?!?

@Flyingmana
Copy link
Contributor Author

will look into it

@Flyingmana Flyingmana force-pushed the Flyingmana-patch-1 branch 2 times, most recently from 1436b70 to cc5015a Compare December 30, 2022 20:23
@github-actions

This comment has been minimized.

Comment on lines 23 to 24
schedule:
- cron: '33 4 * * 4'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Set path to js only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the cron trigger.
We dont have any js files outside the js directory? then makes sense to add it, too

Copy link
Contributor

Choose a reason for hiding this comment

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

we've a lot of .js in /skin/

can it also check the inline js in templates?

Copy link
Contributor

@sreichel sreichel Dec 30, 2022

Choose a reason for hiding this comment

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

I'd use include **/*.js instead of ... to run it only when js files changed.

    paths-ignore:
      - '**/*.md'
      - '**/*.txt'

@github-actions

This comment has been minimized.

@sreichel
Copy link
Contributor

Nice, LGTM.

I'd add it to current workflow later.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 30, 2022

Unit Test Results

7 tests   5 ✔️  0s ⏱️
1 suites  2 💤
1 files    0

Results for commit 71da6e1.

♻️ This comment has been updated with latest results.

@addison74
Copy link
Contributor

@Flyingmana - Nice work and super fast.

@Flyingmana
Copy link
Contributor Author

I now got all errors covered with paths-ignore.
Baseline is a bit more complicated I think, as the state of errors is stored in github, where we can mark the alerts as ignored then one by one.

@fballiano
Copy link
Contributor

Could we test the js in templates?

@Flyingmana
Copy link
Contributor Author

Could we test the js in templates?

iam not sure, first it should replace the LGTM what we had before. I dont think it was able to parse the templates.

But it might be able with codeql, although its a bit more work, why I would delay it for a later time

@sreichel
Copy link
Contributor

But it might be able with codeql, although its a bit more work

If we have a working js code coverage ... 👍

@sreichel sreichel mentioned this pull request Dec 31, 2022
10 tasks
@fballiano fballiano merged commit 43037c1 into 1.9.4.x Dec 31, 2022
@fballiano fballiano deleted the Flyingmana-patch-1 branch December 31, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace LGTM.com as it's going to shutdown in Dec. 2022
4 participants