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 packs input to init action #591

Merged
merged 3 commits into from
Jun 25, 2021
Merged

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Jun 23, 2021

This input allows users to specify which packs to run. It works in
unison with the packs block of the config file and it is similar to
how queries works. They both use + in the same way.

Note that the #TODO in the pr check is still around, but the CLI
is available. I will remove the TODO in the next commit.

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.

@aeisenberg aeisenberg requested a review from a team as a code owner June 23, 2021 22:42
This input allows users to specify which packs to run. It works in
unison with the packs block of the config file and it is similar to
how `queries` works. They both use `+` in the same way.

Note that the `#TODO` in the pr check is still around, but the CLI
is available. I will remove the TODO in the next commit.
Copy link
Contributor

@edoardopirovano edoardopirovano left a comment

Choose a reason for hiding this comment

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

Nice! This mostly looks good, a few comments inline.

.github/workflows/pr-checks.yml Show resolved Hide resolved
.github/workflows/pr-checks.yml Show resolved Hide resolved
.github/workflows/pr-checks.yml Show resolved Hide resolved
init/action.yml Outdated Show resolved Hide resolved
src/config-utils.test.ts Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
if (packsInput.startsWith("+")) {
packsInput = packsInput.substring(1).trim();
if (!packsInput) {
throw new Error("Remove the '+' from the packs input.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a little more informative, e.g. A + was used in the packs input to specify that you wished to add some packs to your CodeQL analysis. However, no packs were specified. Please either remove the + or specify some packs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aeisenberg Think you missed this comment? Unless you disagree, in which case no need to address it 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Missed this. Thanks for pointing it out.

src/runner.ts Outdated Show resolved Hide resolved
.github/workflows/pr-checks.yml Show resolved Hide resolved
- uses: ./../action/init
with:
config-file: ".github/codeql/codeql-config-packaging3.yml"
packs: dsp-testing/codeql-pack1@0.0.4
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need a + here? Otherwise this LGTM now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh!

@aeisenberg aeisenberg force-pushed the aeisenberg/pack-in-inputs branch 2 times, most recently from 257ff39 to 76e2e21 Compare June 25, 2021 16:36
@aeisenberg
Copy link
Contributor Author

Build is failing because the version of the cli it is grabbing from the toolcache is not yet new enough to support all of the packaging commands.

Also, update the options and inputs documentation.
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.

2 participants