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

Feat: Add KMS methods for Key Rotations and MACs #8462

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MauriceBrg
Copy link
Contributor

@MauriceBrg MauriceBrg commented Jan 3, 2025

Implemented New Methods for KMS:

  • generate_mac
  • rotate_key_on_demand
  • list_key_rotations
  • verify_mac

Added these already implemented methods as dummies to the Backend so they show up in the coverage:

  • list_key_policies
  • generate_random

(When I ran scripts/implementation_coverage.py it also updated a bunch of other rst-files (>50) with new methods, I'm not sure if I should commit those as well, as most are unrelated to KMS)

Implemented New Methods:

- rotate_key_on_demand
- list_key_rotations

Added these already implemented methods as dummies to the Backend so they show up in the coverage:

- list_key_policies
- generate_random
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 95.34884% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.63%. Comparing base (ec277aa) to head (70a4eb3).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
moto/kms/models.py 92.85% 2 Missing ⚠️
moto/kms/responses.py 95.12% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #8462    +/-   ##
========================================
  Coverage   92.62%   92.63%            
========================================
  Files        1225     1225            
  Lines      105655   105792   +137     
========================================
+ Hits        97865    97996   +131     
- Misses       7790     7796     +6     
Flag Coverage Δ
servertests 27.75% <30.23%> (-0.01%) ⬇️
unittests 92.60% <95.34%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MauriceBrg
Copy link
Contributor Author

The two lines missing coverage are for the dummy methods to include list_key_policies and generate_random in the implementation coverage. Should I write some dummy tests or call them from the responses?

moto/kms/models.py Outdated Show resolved Hide resolved
moto/kms/models.py Outdated Show resolved Hide resolved
@bblommers
Copy link
Collaborator

Hi @MauriceBrg, thanks for the PR! Just left two comments.

(When I ran scripts/implementation_coverage.py it also updated a bunch of other rst-files (>50) with new methods, I'm not sure if I should commit those as well, as most are unrelated to KMS)

That will be either because Moto hasn't been updated in a while, or you're running the script with an outdated version of botocore. Either way, it's not necessary - the implementation docs will always be updated when we release a new version.

The two lines missing coverage are for the dummy methods to include list_key_policies and generate_random in the implementation coverage. Should I write some dummy tests or call them from the responses?

I'm OK with the current approach. Having 100% coverage is a nice to have, but in the end, it doesn't functionally change anything. We have an implementation, and we have tests for it - that's all I care about.

…E.md and simplify the verify_mac implementation
@MauriceBrg
Copy link
Contributor Author

Hey @bblommers

thanks for the suggestions, I have updated the code to address them and removed the update IMPLEMENTATION_COVERAGE.md for this Pull Request :)

I've updated boto3 and botocore, now the scripts/implementation_coverage.py would only update a handful of files, but I'll leave that to you for the next release.

boto3==1.35.92
botocore==1.35.92

@MauriceBrg MauriceBrg changed the title Feat: Add KMS methods for Key Rotations Feat: Add KMS methods for Key Rotations and MACs Jan 6, 2025
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.

2 participants