website
website copied to clipboard
Epic: Enable code scanning on JS files
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
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
- [ ] Add the following action item to the bottom of the issue
- [ ] Modify the above code files to remove non-JS statements. The preferred approach would be to move liquid statements into HTML. We should start with a single, simpler JS code file in order to prove that the approach will be effective. See #5258
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:
@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.
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
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
@roslynwythe I think this is good. Do you want me to prioritize?
@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 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
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 :)
- Created #6548
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 :)
@gaylem @ExperimentsInHonesty see
- https://github.com/hackforla/website/issues/6548#issuecomment-2040790973
- https://github.com/hackforla/website/issues/6548#issuecomment-2041393563
@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 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.
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