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

PLATFORM-3565: add detect-secrets formula #13

Merged
merged 2 commits into from Sep 8, 2021

Conversation

ovasdi
Copy link
Contributor

@ovasdi ovasdi commented Sep 8, 2021

https://artsyproduct.atlassian.net/browse/PLATFORM-3565

As part of piloting a new process for detect-secrets it would be really helpful if instead of having to direct contributors to https://github.com/artsy/potential to install the modified formula we could just run brew install ... which would steamline rolling this tool out across the org.

@jonallured
Copy link
Member

This looks correct in terms of adding to the formulas in the repo but I was a bit confused on the context. I ended up following through some of those links in your description and hit this one: https://github.com/artsy/potential/pull/414.

And this really helped me understand the problem:

The main developer is no longer working at Yelp and so it may be some time before this bug is addressed. In the mean time, we could instead use a modified version of the official formulae and edit the python version to be python@3.7 which seems to solve the issue. This is better IMO to using the previous release, 1.0.3, and miss out on fixes and latest features.

So we have modified the formula temporarily so that we can use brew to install it and this PR just moves the modified formula to our "official" tap. Did I get this all right?

If I'm understanding correctly then while we're in this temporary state we essentially have a fork of the official version and it would be a manual process of checking on the official one for updates and then applying those changes here, right? Seems somewhat risky? Maybe we merge this and set a reminder to revisit in a month or so? Wishing there was a better way here.

@ovasdi
Copy link
Contributor Author

ovasdi commented Sep 8, 2021

So we have modified the formula temporarily so that we can use brew to install it and this PR just moves the modified formula to our "official" tap. Did I get this all right?

Yea, that is exactly right. To expand a bit more on that, If we keep it in potential and use the --build-from-source flag then if/when the official package gets a release and hopefully the bug is fixed, to upgrade to the latest would just be brew upgrade detect-secrets. Moving to here allows contributors to install from wherever without having depend on potential repo but the tradeoff is that to upgrade we will have to brew uninstall detect_secrets && brew install detect-secrets. Having it in our tap will also prevent from accidentally upgrading to the version that contains the bug.

If I'm understanding correctly then while we're in this temporary state we essentially have a fork of the official version and it would be a manual process of checking on the official one for updates and then applying those changes here, right? Seems somewhat risky? Maybe we merge this and set a reminder to revisit in a month or so? Wishing there was a better way here.

Yep, unfortunately this is the case and if not for that bug with scan copying the formula to here wouldn't be necessary. As far as updates, hopefully the next release will address this issue and we can just move to the official tap.

I agree, having a reminder would help with keeping on top of this 👍. Going to look into this and thanks for suggesting.


RE: IBM fork. I haven't looked into it much lately but I probably should revisit. I wonder if they have a brew package... IIRC the only way to use it was to install via pip and this is not ideal and something we are actively trying to avoid.

@jonallured
Copy link
Member

Ok thanks for writing this up, really helpful context! I'm going to merge this and we can retro on how this went as time passes. This detect-secrets project is pretty cool and helpful I hope Yelp/IBM get their act together and get it to a better state!!

@jonallured jonallured merged commit 6c115c0 into master Sep 8, 2021
@jonallured jonallured deleted the ovasdi/PLATFORM-3565/detect-secrets-formula branch September 8, 2021 18:24
@dblandin
Copy link
Member

dblandin commented Jan 17, 2022

FYI - there has been some recent discussion towards the continued maintenance of detect-secrets. There's even an open PR that addresses the bug this formula works around.

There is also the project Gitleaks (sponsored by Tines that might consider switching to if the maintenance issue persists. Looks like it was recently integrated into GitLab as part of a "secret detection" feature (source).

One meta question - what's the best way to remind ourselves to check in on this in the future? Slack channel reminder?

@ovasdi
Copy link
Contributor Author

ovasdi commented Jan 18, 2022

FYI - there has been some recent discussion towards the continued maintenance of detect-secrets. There's even an open PR that addresses the bug this formula works around.

There is also the project Gitleaks (sponsored by Tines that might consider switching to if the maintenance issue persists. Looks like it was recently integrated into GitLab as part of a "secret detection" feature (source).

One meta question - what's the best way to remind ourselves to check in on this in the future? Slack channel reminder?

@dblandin 👋, thanks for looking into this and for presenting alternatives as well!

To answer the meta question, there is a reminder created in #product-platform that helps us keep an eye on this open issue.

I will take a closer look at the gitleaks tool and how it works but after a quick look at the repo, I wasn't able to determine if the gitleaks tool provides a similar feature to detect-secrets baseline file, which I believe was one of the (key) features that we liked about (detect-secrets) tool when deciding on which tool to adopt. Do you happen to know if gitleaks offers something similar?

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

3 participants