Epic: Enable code scanning on JS files

roslynwythe commented 6 months ago

Dependency

https://github.com/hackforla/website/issues/6548

Overview

Explore options to enable scanning, including the option of modifying the Javascript code to eliminate non-JS statements, as well as the option of performing CodeQL scanning after the Jekyll build.

Details

Many of our Javascript code files cannot be scanned by CodeQL as-is because they contain non-JS (liquid) code which cause extraction errors.

[ ] See issue #5234 for details on the problem. To summarize, these are the problematic code files:

Summary: Non-JS code in these files

hamburger-nav.js: YAML front-matter with a title
toolkit.js: 1 line of Liquid, empty YAML front-matter
wins.js : 2 lines (Liquid), empty YAML front-matter
project.js : 2 lines (Liquid), empty YAML front-matter
about.js: for loop (Liquid), empty YAML front-matter
current-project.js: 2 lines + for loop (Liquid), empty YAML front-matter
Separately, we have observed problems with CodeQL scanning of HTML with embedded liquid statements - see #6485

Action Items

[ ] Create an issue to modify the workflow codeql.yml so that code is scanned after the Jekyll build.

[ ] Add the following action item to the bottom of the issue

- [ ] If this issue was successful in allowing us to us to perform CodeQL scanning after the Jekyll build, close the epic #6378 with a comment indicating that this issue (include issue number) was successful at resolving the requirement
- [ ] If this issue is not successful, move the epic #6378 from the icebox to the new issue approval column, check off the dependency, remove dependency label, add the ready for dev lead label.

[ ] Add the issue you just created as a dependency on this issue, add dependency label, and move this issue to the icebox and unassign yourself

[ ] If the approach outlined in the previous action item is not feasible, then we could modify the above code files to remove non-JS statements. In some instances, liquid/YAML can be moved into HTML. See #5258 in which changes were made to segregate liquid from js in order to avoid ESLint errors.

github-actions[bot] commented 6 months ago

Hi @roslynwythe.

Please don't forget to add the proper labels to this issue. Currently, the labels for the following are missing:

Complexity, Role, Feature, Size

NOTE: Please ignore this comment if you do not have 'write' access to this directory.

To add a label, take a look at Github's documentation here.

Also, don't forget to remove the "missing labels" afterwards. To remove a label, the process is similar to adding a label, but you select a currently added label to remove it.

After the proper labels are added, the merge team will review the issue and add a "Ready for Prioritization" label once it is ready for prioritization.

Additional Resources:

roslynwythe commented 6 months ago

@ExperimentsInHonesty I listed several options under "Options to Explore". I'm thinking perhaps we should write a back end/devOps issue to explore the first option (of modifying the GitHub action), and also one or more frontend issues to explore the other options. Please advise.

ExperimentsInHonesty commented 6 months ago

RW and I talked about this issue via zoom. It was decided that:

an issue needs to be written to try to have the CodeQL scan after Jekyll build and if that's feasible and does not have any unwanted side effects, then we can consider this issue closed
if scanning after build is not feasible, make issues to separate the liquid to html files.

roslynwythe commented 6 months ago

@ExperimentsInHonesty an Action Items section was added to reflect those changes

RW and I talked about this issue via zoom. It was decided that:

an issue needs to be written to try to have the CodeQL scan after Jekyll build and if that's feasible and does not have any unwanted side effects, then we can consider this issue closed

if scanning after build is not feasible, make issues to separate the liquid to html files.

@ExperimentsInHonesty an Action Items section was added to reflect those changes

ExperimentsInHonesty commented 6 months ago

@roslynwythe I think this is good. Do you want me to prioritize?

ExperimentsInHonesty commented 6 months ago

@roslynwythe I rewrote the action items a little bit so the epic can get closed automatically if the first issue is successful. If it looks good to you, add back the ready for prioritization label.

roslynwythe commented 6 months ago

@roslynwythe I rewrote the action items a little bit so the epic can get closed automatically if the first issue is successful. If it looks good to you, add back the ready for prioritization label.

Yes that is good

github-actions[bot] commented 5 months ago

Hi @gaylem, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

gaylem commented 5 months ago

Created #6548

github-actions[bot] commented 5 months ago

Hi @roslynwythe, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

roslynwythe commented 5 months ago

@gaylem @ExperimentsInHonesty see

ExperimentsInHonesty commented 5 months ago

@roslynwythe Sorry, I should have asked you about this one when we were online today.

So does this issue get closed, or do you still think it might be viable if the two options in #6548 are not successful?

roslynwythe commented 4 months ago

@roslynwythe Sorry, I should have asked you about this one when we were online today.

So does this issue get closed, or do you still think it might be viable if the two options in Update codeql.yml to exclude YAML front-matter and Liquid code #6548 are not successful?

@ExperimentsInHonesty - you are correct, if #6548 is not successful, we will have to look for other solutions. @gaylem advised that the first item I listed under "Action Items" would not work because it is not possible to "delay" CodeQL scanning until after the Jekyll build. I believe that the second Action Item is still valid although it will require some research before writing issues.

ExperimentsInHonesty commented 4 months ago

I have removed the ready for labels... and when the dependency is satisfied, this issue will get looked at again by the merge team in the new issue approval column

hackforla / website