FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add new configuration Parameter by tgrall · Pull Request #1590 · github/codeql-action · GitHub

/ codeql-action Public

Add new configuration Parameter - #1590

Closed
tgrall wants to merge 18 commits into
github:mainfrom
tgrall:issue-1589-config-param
ConversationCommits (18)ChecksFiles changed
Closed

Add new configuration Parameter#1590
tgrall wants to merge 18 commits into
github:mainfrom
tgrall:issue-1589-config-param

Conversation

tgrall commented Mar 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Write test to check it is read from configuration
  • Update documentation

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.
  • I may have to work on some exception management when the configuration parameter is not well formatted.

close #1589 1589

- Add new configuration Parameter
ee44252 
- Write test to check it is read from configuration
- Update documentation
tgrall requested a review from a team as a code owner March 18, 2023 13:47
tgrall changed the title - Add new configuration Parameter Add new configuration Parameter Mar 18, 2023
aibaars mentioned this pull request Mar 21, 2023
Closed
tgrall dismissed stale reviews from ghost via fe4a785 April 1, 2023 05:13

tgrall commented Apr 1, 2023

Copy link
Copy Markdown
Contributor Author

@henrymercer @AlonaHlobina : I have renamed the action parameter to config to match the config-file one

henrymercer reviewed Apr 4, 2023
View reviewed changes

henrymercer left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for your contribution! Some suggestions to docs and code.

Comment thread CHANGELOG.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread init/action.yml Outdated
Comment thread src/config-utils.ts Outdated
Comment thread src/config-utils.ts Outdated
Comment thread src/init.ts Outdated
Comment thread README.md Outdated
tgrall and others added 8 commits April 10, 2023 07:33
Merge branch 'main' into issue-1589-config-param
18f1345
Accept change in PR
696504d 
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Update init/action.yml : PR review
a9fb7d9 
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Update README.md : during PR Review
d2950c1 
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Update README.md : during PR Review
c9f360d 
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Update CHANGELOG.md : during PR Review
0b75f47 
Co-authored-by: Henry Mercer <henry.mercer@me.com>
resolve comments from @henrymercer
b4e6f81
remove the documentation about workflow parameters
fc374f5
tgrall requested a review from henrymercer April 10, 2023 07:23

tgrall commented Apr 10, 2023

Copy link
Copy Markdown
Contributor Author

@henrymercer

  • thanks for the review, I have applied your changes.
  • I see many issues in the workflows, that were not present before, not sure what is the issue
  • I was not able to do my usual manual e2e testing, as my fork cannot get the CLI 2.12.6 (it was not the case before this release)

henrymercer reviewed Apr 14, 2023
View reviewed changes

henrymercer left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks — this looks good, just some minor followup comments. Your PR checks failed due to hitting API rate limits. They should hopefully pass on a retry. If not, we can try to find a workaround. I am not sure why your fork cannot get CodeQL CLI 2.12.6. If you're still hitting this error, please provide us an error message and repro steps so we can investigate.

Comment thread README.md Outdated
Comment thread src/config-utils.test.ts Outdated
Comment thread src/config-utils.test.ts Outdated
Comment thread src/config-utils.test.ts
const inputFileContents = `
name: my config
queries:
- uses: ./foo

henrymercer Apr 14, 2023

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Minor: It'd be clearer to extract foo into a variable so we can see more easily why we're creating the foo directory and referencing foo later on.

Comment thread src/config-utils.test.ts Outdated
Comment thread src/config-utils.ts
henrymercer reviewed Apr 18, 2023
View reviewed changes

henrymercer left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for addressing my comments. Could you merge in main and rebuild so we can run the PR checks?

jeffwidman mentioned this pull request Apr 20, 2023
Merged

jeffwidman commented Apr 20, 2023

Copy link
Copy Markdown
Member

I assume this will also support the paths-ignore key?

Over in we'd like to configure CodeQL to ignore two files, and seemed like overkill to have to create the indirection of a separate config file just to specify those two files...

jeffwidman mentioned this pull request Apr 20, 2023
Closed
henrymercer mentioned this pull request Apr 21, 2023
Closed
3 tasks

henrymercer commented Apr 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@jeffwidman 👋 Yes, all keys supported by the config file, including paths-ignore, will be supported.

aliscco mentioned this pull request Apr 28, 2023
Open

aeisenberg commented Apr 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

What's the status of this PR? Are we just waiting for a merge with main?

I don't have push access to this fork, or else I would do it.

henrymercer commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

If the PR checks pass, I'm happy with the PR. @tgrall Would you mind merging in main and rebuilding the Action so we can run the checks and merge?

aeisenberg mentioned this pull request May 1, 2023
Merged
3 tasks

aeisenberg commented May 1, 2023

Copy link
Copy Markdown
Contributor

I'll take this PR over. Superseded by #1665.

aeisenberg closed this May 1, 2023
coderabbitai Bot mentioned this pull request Mar 6, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

henrymercer henrymercer left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Provide a way to pass a whole configuration as parameter

4 participants

Back | FazBrowse Home | New Git URL