website icon indicating copy to clipboard operation
website copied to clipboard

Epic: Enable code scanning on JS files

Open roslynwythe opened this issue 1 year ago • 7 comments

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
  • [ ] 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

roslynwythe avatar Feb 26 '24 04:02 roslynwythe

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:

github-actions[bot] avatar Feb 26 '24 04:02 github-actions[bot]

@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.

roslynwythe avatar Feb 28 '24 00:02 roslynwythe

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 avatar Mar 03 '24 17:03 ExperimentsInHonesty

@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 avatar Mar 03 '24 18:03 roslynwythe

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

ExperimentsInHonesty avatar Mar 04 '24 02:03 ExperimentsInHonesty

@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.

ExperimentsInHonesty avatar Mar 08 '24 21:03 ExperimentsInHonesty

@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

roslynwythe avatar Mar 11 '24 16:03 roslynwythe

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 :)

github-actions[bot] avatar Mar 29 '24 20:03 github-actions[bot]

  • Created #6548

gaylem avatar Mar 29 '24 22:03 gaylem

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 :)

github-actions[bot] avatar Apr 05 '24 19:04 github-actions[bot]

@gaylem @ExperimentsInHonesty see

  • https://github.com/hackforla/website/issues/6548#issuecomment-2040790973
  • https://github.com/hackforla/website/issues/6548#issuecomment-2041393563

roslynwythe avatar Apr 07 '24 16:04 roslynwythe

@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?

ExperimentsInHonesty avatar Apr 24 '24 00:04 ExperimentsInHonesty

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

  • @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.

roslynwythe avatar Apr 28 '24 07:04 roslynwythe

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

ExperimentsInHonesty avatar May 03 '24 23:05 ExperimentsInHonesty