Skip to content

Conversation

simon-engledew
Copy link

@simon-engledew simon-engledew commented Nov 4, 2020

I've added a bare bones implemention of the codeql-path output variable. Opening this PR to get feedback and check it is aligned with expectations. 👍

  • Expand readme documentation
  • See if there is a better way of testing other than implicitly via the python-deps workflow (– added a second use of the output. Happy that the build will break if there is a regression.)

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@simon-engledew simon-engledew force-pushed the simon-engledew/output-codeql-path branch from 233de10 to 77f914a Compare November 4, 2020 17:23
Copy link
Contributor

@chrisgavin chrisgavin left a comment

Choose a reason for hiding this comment

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

👍 Awesome. It ended up being an even smaller change than I hoped and it seems like people will find it useful.

@simon-engledew simon-engledew force-pushed the simon-engledew/output-codeql-path branch from a09a301 to f1e2256 Compare November 4, 2020 19:16
@simon-engledew simon-engledew force-pushed the simon-engledew/output-codeql-path branch from 3951135 to 68ca540 Compare November 4, 2020 19:29
Also add example from README into workflow to confirm it is accurate.
@simon-engledew simon-engledew force-pushed the simon-engledew/output-codeql-path branch from 68ca540 to c87f302 Compare November 4, 2020 19:35
@simon-engledew
Copy link
Author

simon-engledew commented Nov 4, 2020

I can't see an obvious way to test this feature other than the pair of integration tests I've added. The build will break if this feature stops working for any reason - is that good enough coverage?

I think we have consensus that this is enough testing for now 👍

I'm not super happy with the README changes. It feels like something is missing or the tone doesn't quite match the copy above. Happy to take suggestions for better wording. ✍️

Also I reference init@v1 which wont have this output. Should I reference init@main or make the current copy true by eventually backporting this change?

This can move into the documentation rather than the README

As this is an advanced usage it makes more sense to work to getting this included in the documentation instead.
@simon-engledew simon-engledew marked this pull request as ready for review November 5, 2020 08:32
Copy link
Contributor

@chrisgavin chrisgavin left a comment

Choose a reason for hiding this comment

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

Yep, I can see this is probably hard to test directly without it just being an assertion that codeQLCmd is correct, which is already tested in a bunch of other places.

Having the test in the workflow is pretty nice, even if it's not very explicit about what it's testing.

@@ -19,7 +19,7 @@ jobs:
with:
languages: javascript
config-file: ./.github/codeql/codeql-config.yml
# example from README.md
# confirm steps.init.outputs.codeql-path points to the codeql binary
Copy link
Author

Choose a reason for hiding this comment

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

I have left a comment of intent to hopefully make it more clear why this step is in the workflow 👍

@simon-engledew simon-engledew merged commit f13bd45 into main Nov 5, 2020
@simon-engledew simon-engledew deleted the simon-engledew/output-codeql-path branch November 5, 2020 10:22
@github-actions github-actions bot mentioned this pull request Nov 9, 2020
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.

3 participants