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

Include AWS Tags in Resource Generation #4998

Merged
merged 26 commits into from
Jul 18, 2024
Merged

Conversation

tariqajyusuf
Copy link
Contributor

@tariqajyusuf tariqajyusuf commented Jun 18, 2024

Closes #4997

Description Of Changes

Includes AWS tags as meta fields in resource generation.

Code Changes

  • Fix typing error in storage.py
  • Change get_tagging_resources to return a full list of resources including tags. Also change downstream flows.
  • Add AWS tags to Fides meta

Steps to Confirm

  • Start dev environment.
  • Attempt to scan an AWS environment with tagged resources.

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation:
    • documentation complete, PR opened in fidesdocs
    • documentation issue created in fidesdocs
    • if there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated
  • If there are any database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!

Copy link

vercel bot commented Jun 18, 2024

@tariqajyusuf is attempting to deploy a commit to the Ethyca Team on Vercel.

A member of the Team first needs to authorize it.

@rbart
Copy link

rbart commented Jun 18, 2024

yes please!

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 27.27273% with 8 lines in your changes missing coverage. Please review.

Project coverage is 86.47%. Comparing base (e63a74f) to head (7ea79b0).
Report is 7 commits behind head on main.

Files Patch % Lines
src/fides/connectors/aws.py 22.22% 7 Missing ⚠️
src/fides/core/system.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4998      +/-   ##
==========================================
- Coverage   86.50%   86.47%   -0.04%     
==========================================
  Files         357      357              
  Lines       22302    22272      -30     
  Branches     2946     2946              
==========================================
- Hits        19293    19260      -33     
- Misses       2495     2497       +2     
- Partials      514      515       +1     

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

Copy link
Contributor

@SteveDMurphy SteveDMurphy 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 making this happen @tariqajyusuf as well as for the patience with the review time!!

There are a couple of items that may need a closer look that I've commented on around handling, (Tags and TagList not existing) but I was also curious if we wanted to also look for tags that could be found on Dynamo Datasets. Would it be value-added to do those now in your eyes as well?

Outside of those changes, can you also make a Changelog entry under unreleased? Happy to jump in and help out as well if you'd like! Apologies I was unable to try and catch up with you synchronously this evening.

src/fides/connectors/aws.py Outdated Show resolved Hide resolved
src/fides/connectors/aws.py Outdated Show resolved Hide resolved
@tariqajyusuf
Copy link
Contributor Author

I added some checks and made sure that they don't break the tests. I'm still getting a strange issue with an unrelated test tests/ctl/core/test_api.py::TestSystemUpdate::test_system_update_system_cookies and I'm stumped as to why.

On the DynamoDB dataset comment, I think that could make sense to include that but I think that will require some propagating down since the describe_table call from boto3 doesn't return resource tags. That will require a little more bandwidth than I currently have right now.

Updated the changelog!

Copy link
Contributor

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

Looks great @tariqajyusuf - thanks for making those changes and getting this added in!

@SteveDMurphy SteveDMurphy merged commit 8113867 into ethyca:main Jul 18, 2024
16 of 17 checks passed
Copy link

cypress bot commented Jul 18, 2024

Passing run #9011 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Include AWS Tags in Resource Generation (#4998)
Project: fides Commit: 8113867785
Status: Passed Duration: 00:37 💡
Started: Jul 18, 2024 8:48 PM Ended: Jul 18, 2024 8:48 PM

Review all test suite changes for PR #4998 ↗︎

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.

Include AWS tags as Fides tags in resource generation
4 participants