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 threat-models as a property to config file and inputs #1653

Closed
wants to merge 1 commit into from

Conversation

aeisenberg
Copy link
Contributor

There's a lot of changes here, but it's pretty formulaic. It follows the approach used by the queries input and config property. threat-models can appear as an input or in the config file. If it appears in the input, then we need to either merge it with the threat-models in the config (if prefixed with +) or overwrite it.

There's no danger if someone uses threat-models with an older CLI since the CLI can handle configs with extra properties.

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 April 19, 2023 22:02
@aeisenberg aeisenberg added the Update dependencies Trigger PR workflow to update dependencies label Apr 20, 2023
There's a lot of changes here, but it's pretty formulaic. It follows the
approach used by the `queries` input and config property.
`threat-models` can appear as an input or in the config file. If it
appears in the input, then we need to either merge it with the
threat-models in the config (if prefixed with `+`) or overwrite it.

There's no danger if someone uses `threat-models` with an older CLI
since the CLI can handle configs with extra properties.
@github-actions github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Apr 20, 2023
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.

Another approach here would be to merge the work on allowing users to pass config properties via a config input (#1590), then letting users pass threat models via something like:

uses: github/codeql-action/init@v2
with:
  config: |
    threat-models: <threat models>

An advantage of this is consistency — we have a single way to pass in threat models. Another is less special-purpose code — it would be nice to be able to handle inputs like these in the CLI without having to add code to handle them in the Action too. A disadvantage is that users couldn't use a config file but then also override or add some threat models. I'm not sure how important that is.

@aeisenberg
Copy link
Contributor Author

Thanks good point. It keeps the code significantly simpler.

My only concern about this is the inconsistency. We allow users to specify packs and queries via inputs. Would we not want to do the same with threat models?

Perhaps the best approach would be to explicitly deprecate the packs and queries inputs (and any others that are duplicated in the config file).

@aeisenberg
Copy link
Contributor Author

Short term, I will not be pursuing this since there is a way to use threat models with no changes to the action.

If we decide later that we want to add a specific input for threat models, then I can start working on this again.

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.

None yet

2 participants