Skip to content

Add new configuration Parameter #1590

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.

Sign up for GitHub

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

Jump to bottom
Closed
tgrall wants to merge 18 commits into from
Closed

Add new configuration Parameter #1590

tgrall wants to merge 18 commits into from

Conversation

@tgrall
Copy link
Contributor

@tgrall tgrall commented Mar 18, 2023 •
edited
Loading

  • 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

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

tgrall commented Apr 1, 2023

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

henrymercer
henrymercer reviewed Apr 4, 2023
View reviewed changes
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

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

@tgrall tgrall requested a review from henrymercer April 10, 2023 07:23
@tgrall
Copy link
Contributor Author

tgrall commented Apr 10, 2023

@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
henrymercer reviewed Apr 14, 2023
View reviewed changes
Copy link
Contributor

@henrymercer henrymercer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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.

src/config-utils.test.ts
const inputFileContents = `
name: my config
queries:
- uses: ./foo
Copy link
Contributor

@henrymercer henrymercer Apr 14, 2023

Choose a reason for hiding this comment

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

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.

henrymercer
henrymercer reviewed Apr 18, 2023
View reviewed changes
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

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

@jeffwidman jeffwidman mentioned this pull request Apr 20, 2023
Merged
@jeffwidman
Copy link
Member

jeffwidman commented Apr 20, 2023

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

Over in :dependabot: 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 jeffwidman mentioned this pull request Apr 20, 2023
Closed
@henrymercer henrymercer mentioned this pull request Apr 21, 2023
Closed
3 tasks
@henrymercer
Copy link
Contributor

henrymercer commented Apr 21, 2023 •
edited
Loading

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

@aliscco aliscco mentioned this pull request Apr 28, 2023
Open
@aeisenberg
Copy link
Contributor

aeisenberg commented Apr 28, 2023 •
edited
Loading

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
Copy link
Contributor

henrymercer commented Apr 28, 2023

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 aeisenberg mentioned this pull request May 1, 2023
Merged
3 tasks
@aeisenberg
Copy link
Contributor

aeisenberg commented May 1, 2023

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

@aeisenberg aeisenberg closed this May 1, 2023
@coderabbitai coderabbitai bot mentioned this pull request Mar 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@tgrall @jeffwidman @henrymercer @aeisenberg