Skip to content

pre-commit: add hook to check for the existence of private keys#7805

Merged
DaanHoogland merged 1 commit into
apache:mainfrom
jbampton:add-hook-detect-private-key
Aug 20, 2023
Merged

pre-commit: add hook to check for the existence of private keys#7805
DaanHoogland merged 1 commit into
apache:mainfrom
jbampton:add-hook-detect-private-key

Conversation

@jbampton

@jbampton jbampton commented Aug 2, 2023

Copy link
Copy Markdown
Member

https://github.com/pre-commit/pre-commit-hooks#detect-private-key

The "exclude" list is the list of "keys" currently found by this hook

Description

This PR adds another pre-commit hook or "test" to check for "keys" in the codebase

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

You can test locally with pre-commit from the repo root:

pre-commit run --all-files

To just test this hook run:

pre-commit run detect-private-key --all-files

@codecov

codecov Bot commented Aug 2, 2023

Copy link
Copy Markdown

Codecov Report

Merging #7805 (e1c115a) into main (6f45034) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #7805      +/-   ##
============================================
- Coverage     13.44%   13.44%   -0.01%     
+ Complexity     9400     9396       -4     
============================================
  Files          2747     2747              
  Lines        258844   258844              
  Branches      40313    40313              
============================================
- Hits          34804    34790      -14     
- Misses       219655   219674      +19     
+ Partials       4385     4380       -5     

see 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jbampton

jbampton commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@jbampton a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good, not sure if the console proxy should be excluded though

@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6640

@jbampton

jbampton commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

looks good, not sure if the console proxy should be excluded though

Hey @DaanHoogland when you run this new hook for the first time it finds that list of keys in the codebase.

So I was then excluding them assuming they were all valid keys that we need.

@DaanHoogland

Copy link
Copy Markdown
Contributor

looks good, not sure if the console proxy should be excluded though

Hey @DaanHoogland when you run this new hook for the first time it finds that list of keys in the codebase.

So I was then excluding them assuming they were all valid keys that we need.

yeah, makes sense and I have no doubts about most (systemvm and tests) . I'll need to look at the consoleproxy one.

@DaanHoogland

Copy link
Copy Markdown
Contributor

looks good, not sure if the console proxy should be excluded though

Hey @DaanHoogland when you run this new hook for the first time it finds that list of keys in the codebase.
So I was then excluding them assuming they were all valid keys that we need.

yeah, makes sense and I have no doubts about most (systemvm and tests) . I'll need to look at the consoleproxy one.

false alarm (didn't notice this is in a test dir as well)

@stephankruggg stephankruggg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@DaanHoogland DaanHoogland merged commit 0b4afed into apache:main Aug 20, 2023
@jbampton jbampton deleted the add-hook-detect-private-key branch August 20, 2023 16:13
DaanHoogland added a commit to DaanHoogland/cloudstack that referenced this pull request Aug 24, 2023
@DaanHoogland DaanHoogland added this to the 4.19.0.0 milestone Sep 29, 2023
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.

4 participants