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

Resolve CodeQL extraction errors #5234

Closed
2 tasks done
Tracked by #5005
roslynwythe opened this issue Aug 15, 2023 · 8 comments
Closed
2 tasks done
Tracked by #5005

Resolve CodeQL extraction errors #5234

roslynwythe opened this issue Aug 15, 2023 · 8 comments
Assignees
Labels
Complexity: Medium Feature: Code Alerts ready for dev lead Issues that tech leads or merge team members need to follow up on role: back end/devOps Tasks for back-end developers size: 1pt Can be done in 4-6 hours

Comments

@roslynwythe
Copy link
Member

roslynwythe commented Aug 15, 2023

Overview

The CodeQL workflow GHA is reporting that it cannot scan 6 files in the gh-pages branch of hackforla/website due to extraction errors, presumably due to syntax errors. We need to resolve those errors to ensure complete testing coverage.

Action Items

  • Examine the CodeQL workflow logs in depth in order to understand the extraction and syntax errors.
  • Resolve the errors or advise dev leads/merge team why the errors cannot be resolved.

Resources/Instructions

GitHub CodeQL documentation.
workflow run logs for codeql.yml

@roslynwythe roslynwythe added role: back end/devOps Tasks for back-end developers Complexity: Medium size: 1pt Can be done in 4-6 hours Feature: Code Alerts Draft Issue is still in the process of being created Ready for Prioritization and removed Draft Issue is still in the process of being created labels Aug 15, 2023
@anjolaaoluwa anjolaaoluwa added this to the x. Technical debt milestone Aug 27, 2023
@roslynwythe roslynwythe removed the Draft Issue is still in the process of being created label Nov 7, 2023
@elliot-d-kim elliot-d-kim self-assigned this Feb 7, 2024
Copy link

github-actions bot commented Feb 7, 2024

Hi @elliot-d-kim, 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 :)

@elliot-d-kim
Copy link
Member

Availability: Mon-Thu outside of 2-7pm, Fri before 2pm, Sat all day
ETA: 2/13/24

@elliot-d-kim
Copy link
Member

Progress Update

  1. Progress: Discovered all 6 syntax errors raised by CodeQL appear to be related to YAML front matter in .js files under /assets/js.
  2. Blockers: Planning to test CodeQL scan against changes in forked repo, but awaiting clarification from @roslynwythe on first-time set up of CodeQL for testing purposes.
  3. Availability: Mon-Thu outside of 2-7pm, Fri before 2pm, Sat all day
  4. ETA: 2/16/24

@roslynwythe
Copy link
Member Author

@elliot-d-kim Please detail the files involved and whether they have liquid statements or just YAML front matter that is causing the problem with CodeQL. Thanks!

@elliot-d-kim
Copy link
Member

elliot-d-kim commented Feb 17, 2024

"Syntax errors" prevent CodeQL from scanning .js files

It seems that the current CodeQL configuration can't scan JS files when YAML, Liquid, etc. is mixed in. The error message "Could not process some files due to syntax errors" sounds to me like these are not simply false alarms of code smells but rather indications that these "syntax errors" prevent CodeQL from scanning the 6 files involved.

Screenshot: CodeQL error message

CodeQL error message 1

The "syntax errors" are non-JS code

I believe the YAML and Liquid in these .js files are responsible. No other files in the .js directory have non-JS code, and CodeQL has no problem scanning those.

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

Testing results

  • Remove only the empty YAML front-matter: CodeQL errors moved down the files to the next non-JS (i.e. Liquid) lines.
  • Remove all non-JS code (YAML, Liquid): CodeQL syntax errors resolve, files properly scanned

I did the latter in total 4 .js files: note that the message goes from "5 other warnings like this" (refer to the very first screenshot) to "1 other" in the following screenshot:

Screenshot: CodeQL error message

CodeQL error message 2

Deleting the Liquid lines would break the site (and CodeQL raised those errors accordingly in testing), so an alternative, holistic solution is required.

Deleting hamburger nav front-matter causes rendering errors in smaller views

Deleting the YAML front-matter in hamburger-nav.js made the file free of non-JS code and made the file completely free of CodeQL errors.

Errors in smaller views: Hamburger nav is not used in larger views but it appears in smaller or mobile views. This is mentioned in comments in _sass/components/_header-nav.scss under /* Hamburger nav CSS */ (line 83). Thus, while it causes no issues in larger views, deleting hamburger-nav.js's non-empty YAML front-matter causes a rendering error in smaller views.

Screenshot: Hamburger nav initial render
Screenshot: Hamburger nav interaction BEFORE deleting front-matter
Screenshot: Hamburger nav interaction AFTER deleting front-matter

@elliot-d-kim elliot-d-kim added the ready for dev lead Issues that tech leads or merge team members need to follow up on label Feb 17, 2024
@roslynwythe
Copy link
Member Author

Great job @elliot-d-kim - your analysis is clear and compelling and supported by your testing. In future issues we will explore various options for solutions. Let me know if you would like to be involved in that work.

@elliot-d-kim
Copy link
Member

Great, I'd definitely be interested in exploring solutions. I'm also curious about the progression of an issue like this up to solution implementation, so please keep me in the loop!

@roslynwythe
Copy link
Member Author

roslynwythe commented Feb 26, 2024

  • @elliot-d-kim For this situation we will require an epic. An epic is a higher-level issue that encompasses the "story" or requirements of multiple issues; that is, it is an issue to write and to track other related issues. I have started working on this: see Epic: Enable code scanning on JS files #6378

If you are interested in working on the epic please contact me via Slack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Medium Feature: Code Alerts ready for dev lead Issues that tech leads or merge team members need to follow up on role: back end/devOps Tasks for back-end developers size: 1pt Can be done in 4-6 hours
Projects
Development

No branches or pull requests

4 participants