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

Flag security alerts and pass versions through #144

Merged
merged 11 commits into from
Feb 22, 2022

Conversation

mwaddell
Copy link
Contributor

closes #84
closes #102

@mwaddell
Copy link
Contributor Author

I still need to add test coverage to the new getAlert method which uses graphQL and update the documentation, but it's pretty close...

@mwaddell mwaddell marked this pull request as ready for review February 19, 2022 04:24
@mwaddell mwaddell requested a review from a team as a code owner February 19, 2022 04:24
@mwaddell
Copy link
Contributor Author

Updated documentation and addressed unit tests

@mwaddell mwaddell changed the title Flag security alerts and pass versions thorugh Flag security alerts and pass versions through Feb 20, 2022
Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you ❤️

I have one thought that might be a bit defensive. This is adds a call to the GitHub API that might be wasted if users don't care about these values - should we use an input flag, which defaults to false to ensure the query is only invoked when explicitly requested? 🤔

I love this pattern of using the API to enrich the metadata further, but it might be worth setting a precedent of doing the least work by default.

@mwaddell
Copy link
Contributor Author

I have one thought that might be a bit defensive. This is adds a call to the GitHub API that might be wasted if users don't care about these values - should we use an input flag, which defaults to false to ensure the query is only invoked when explicitly requested? thinking

I love this pattern of using the API to enrich the metadata further, but it might be worth setting a precedent of doing the least work by default.

Yes, that makes perfect sense - I'll add that and assign it back for review

@mwaddell mwaddell marked this pull request as draft February 21, 2022 16:39
@mwaddell mwaddell marked this pull request as ready for review February 22, 2022 01:20
@mwaddell
Copy link
Contributor Author

Done

Copy link
Contributor

@brrygrdn brrygrdn 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 this @mwaddell! I just want to do a minor release to get some house keeping updates out of the way and I'll get this merged 🚀

@brrygrdn brrygrdn merged commit e35f7ed into dependabot:main Feb 22, 2022
@brrygrdn brrygrdn mentioned this pull request Feb 28, 2022
@mwaddell mwaddell deleted the flag-security-alerts branch March 1, 2022 15:51
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.

Pass version through metadata Flag security related dependabot PRs
2 participants