Skip to content

Update CodeQL workflow and configuration, fix found bugs - #51263

Merged
Jake Bailey (jakebailey) merged 5 commits into
microsoft:mainfrom
jakebailey:update-codeql
Oct 27, 2022
Merged

Update CodeQL workflow and configuration, fix found bugs#51263
Jake Bailey (jakebailey) merged 5 commits into
microsoft:mainfrom
jakebailey:update-codeql

Conversation

@jakebailey

Copy link
Copy Markdown
Member

The config we had here was pretty old and contained some cruft that is no longer recommended by https://github.com/github/codeql-action.

Update it to be based on their new example config (with changes), and ignore src/lib (which does not contain code).

# │ │ │ │ │
# │ │ │ │ │
# * * * * *
- cron: '30 1 * * 0'

ghost Oct 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Love this.

ghost Oct 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank them, I didn't make this 😄

ghost Oct 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still loving it, no matter who added it. I always have to look up the Cron syntax.

ghost Oct 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ghost Oct 22, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why does the minute come first and why are hours 0-based but dates are 1-based? That's confusing as hell.
edit: Wait, 24-hour time uses zero-based hours. Still confusing that the minutes come first, though.


paths:
- './src'
- src

ghost Oct 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we add the scripts directory?

ghost Oct 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe, though that's pure dev dependency; I actually considered ignoring tests too but opted not to.

ghost Oct 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added it and the gulpfile; we'll see if it breaks.

ghost Oct 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks like quite a few things showed up; the ones about uncontrolled user input are probably ignorable since that's what those scripts intend to do anyway. The rest are sort of interesting.

ghost Oct 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One of these actually looks like a bug; the localization script only replaced the first ] when it should have replaced all of them. Of course, what we should do is not produce XML via string templates, but, that's another story. Maybe after modules as my stack includes a build change on that front.

Comment thread scripts/importDefinitelyTypedTests.mjs Fixed
Comment thread scripts/importDefinitelyTypedTests.mjs Fixed
@jakebailey

ghost commented Oct 21, 2022

Copy link
Copy Markdown
Member Author

I made some changes to fix the things that are now found. The only major change is the diagnostic message change, where I've switched us to using JSON.stringify instead, which appears to have no performance difference.

@jakebailey Jake Bailey (jakebailey) changed the title Update CodeQL workflow and configuration Update CodeQL workflow and configuration, fix found bugs Oct 27, 2022
@jakebailey
Jake Bailey (jakebailey) merged commit 754eeb2 into microsoft:main Oct 27, 2022
@jakebailey
Jake Bailey (jakebailey) deleted the update-codeql branch October 27, 2022 23:29
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants