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

Add new configuration Parameter #1590

Closed
wants to merge 18 commits into from

Conversation

tgrall
Copy link
Contributor

@tgrall tgrall commented Mar 18, 2023

  • 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

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

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.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
init/action.yml Outdated Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
src/init.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tgrall and others added 8 commits April 10, 2023 07:33
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Co-authored-by: Henry Mercer <henry.mercer@me.com>
Co-authored-by: Henry Mercer <henry.mercer@me.com>
@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)

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

README.md Outdated Show resolved Hide resolved
src/config-utils.test.ts Outdated Show resolved Hide resolved
src/config-utils.test.ts Outdated Show resolved Hide resolved
const inputFileContents = `
name: my config
queries:
- uses: ./foo
Copy link
Contributor

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.

src/config-utils.test.ts Outdated Show resolved Hide resolved
src/config-utils.ts Show resolved Hide resolved
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
Copy link
Member

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

@henrymercer
Copy link
Contributor

henrymercer commented Apr 21, 2023

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

@aeisenberg
Copy link
Contributor

aeisenberg commented Apr 28, 2023

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

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
3 tasks
@aeisenberg
Copy link
Contributor

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

@aeisenberg aeisenberg closed this May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to pass a whole configuration as parameter
4 participants