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.

Sign up for GitHub

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

Jump to bottom

Create storage credentials based on instance profiles and existing roles. #869

Merged

nfx merged 31 commits into main from feature/migrate-storage-credentials-862

Feb 18, 2024

Contributor

FastLee commented Feb 1, 2024 •

edited

Loading

Changes

Linked issues

relates to #862
closes #913

Resolves #..

Functionality

Added method to detect missing roles and add them to the AWS account.

Screenshare.-.2024-02-17.9_23_51.PM.mp4

Tests

manually tested
added unit tests
added integration tests
verified on staging environment (screenshot attached)

FastLee had a problem deploying to account-admin

February 1, 2024 19:43

— with

GitHub Actions Failure

codecov bot commented Feb 1, 2024 •

edited

Loading

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (228dd8f) 87.92% compared to head (2980285) 87.92%.

Files	Patch %	Lines
src/databricks/labs/ucx/assessment/aws.py	85.88%	5 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
- Coverage   87.92%   87.92%   -0.01%     
==========================================
  Files          43       43              
  Lines        5178     5258      +80     
  Branches      928      943      +15     
==========================================
+ Hits         4553     4623      +70     
- Misses        428      432       +4     
- Partials      197      203       +6

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

nfx requested changes

View reviewed changes

Collaborator

nfx left a comment

please rebase and address comments

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

tests/unit/assessment/test_aws.py Outdated Show resolved Hide resolved

tests/unit/test_cli.py Outdated Show resolved Hide resolved

tests/unit/test_cli.py Outdated Show resolved Hide resolved

tests/unit/test_cli.py Outdated Show resolved Hide resolved

tests/unit/test_cli.py Outdated Show resolved Hide resolved

FastLee force-pushed the feature/migrate-storage-credentials-862 branch from 2148442 to e224cbf Compare

February 16, 2024 01:56

FastLee had a problem deploying to account-admin

February 16, 2024 01:56

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 16, 2024 02:01

— with

GitHub Actions Failure

nfx reviewed

View reviewed changes

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

FastLee had a problem deploying to account-admin

February 16, 2024 14:03

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 16, 2024 15:01

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 16, 2024 18:46

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 16, 2024 19:31

— with

GitHub Actions Failure

nfx requested changes

View reviewed changes

Collaborator

nfx left a comment

Use dictionaries and serialize them to JSON, no string replacement

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

FastLee had a problem deploying to account-admin

February 17, 2024 13:59

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 17, 2024 14:32

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 17, 2024 14:42

— with

GitHub Actions Failure

FastLee marked this pull request as ready for review

February 17, 2024 17:59

FastLee requested review from a team, zpappa and nfx

February 17, 2024 17:59

FastLee added 11 commits

February 17, 2024 13:07


          Initial Commit

2d1e52f


          Validated Scanning

da9a887


          Completed Coding and initial testing.

cf8cb98


          Completed Unit Tests


          Rebasing with roles save

2f81337


          Tested get role actions.

4b24546


          Added missing role functionality.


          Added missing role functionality.

db7a59d


          Prepare for create role cli

84b5422


          Added policy document

dc6921b


          Added policy document

b4d3acd

FastLee had a problem deploying to account-admin

February 17, 2024 19:25

— with

GitHub Actions Failure


          Refactored based on review

ac286eb

FastLee had a problem deploying to account-admin

February 17, 2024 19:40

— with

GitHub Actions Failure

FastLee requested a review from nfx

February 17, 2024 19:41

nfx requested changes

View reviewed changes

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/assessment/aws.py

+                      aws_cmd = shutil.which("aws")
+                      code, _, error = self._command_runner(f"{aws_cmd} {command} --output json")
+                      if code != 0:
+                          logger.error(error)

Collaborator

nfx Feb 17, 2024

Shouldn't we throw the exception instead for it to bubble up to the top?

Contributor Author

FastLee Feb 17, 2024

I'm not sure.


          Addressed review comments

972f784

FastLee had a problem deploying to account-admin

February 17, 2024 20:29

— with

GitHub Actions Failure

FastLee requested a review from nfx

February 17, 2024 20:30

FastLee added 2 commits

February 17, 2024 15:37


          Removed unused function

02388b4


          Removed unused function

c00684d

FastLee had a problem deploying to account-admin

February 17, 2024 20:37

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 17, 2024 20:37

— with

GitHub Actions Failure

FastLee had a problem deploying to account-admin

February 17, 2024 22:14

— with

GitHub Actions Failure

nfx reviewed

View reviewed changes

src/databricks/labs/ucx/assessment/aws.py Outdated Show resolved Hide resolved


          Addressed review comments

d09c5e8

FastLee had a problem deploying to account-admin

February 17, 2024 23:15

— with

GitHub Actions Failure

FastLee requested a review from nfx

February 17, 2024 23:16

FastLee had a problem deploying to account-admin

February 18, 2024 00:05

— with

GitHub Actions Failure

nfx approved these changes

View reviewed changes

Collaborator

nfx left a comment

Add a small demo to PR description and ready to merge


          Fixed integration test

FastLee temporarily deployed to account-admin

February 18, 2024 02:27

— with

GitHub Actions Inactive

FastLee enabled auto-merge

February 18, 2024 11:45

nfx approved these changes

View reviewed changes

nfx merged commit fc48c6f into main

5 of 7 checks passed

nfx deleted the feature/migrate-storage-credentials-862 branch

February 18, 2024 15:18

nfx added a commit that referenced this pull request


          Release v0.13.1

546199b

* Added secret detection logic to Azure service principal crawler ([#950](#950)).
* Create storage credentials based on instance profiles and existing roles ([#869](#869)).
* Enforced `protected-access` pylint rule ([#956](#956)).
* Enforced `pylint` on unit and integration test code ([#953](#953)).
* Enforcing `invalid-name` pylint rule ([#957](#957)).
* Fixed AzureResourcePermissions.load to call Installation.load ([#962](#962)).
* Fixed installer script to reuse an existing UCX Cluster policy if present ([#964](#964)).
* More `pylint` tuning ([#958](#958)).
* Refactor `workspace_client_mock` to have combine fixtures stored in separate JSON files ([#955](#955)).

Dependency updates:

 * Updated databricks-sdk requirement from ~=0.19.0 to ~=0.20.0 ([#961](#961)).

nfx mentioned this pull request

Release v0.13.1 #966

Merged

nfx added a commit that referenced this pull request


          Release v0.13.1 (#966)

d1fdd9d

* Added secret detection logic to Azure service principal crawler
([#950](#950)).
* Create storage credentials based on instance profiles and existing
roles ([#869](#869)).
* Enforced `protected-access` pylint rule
([#956](#956)).
* Enforced `pylint` on unit and integration test code
([#953](#953)).
* Enforcing `invalid-name` pylint rule
([#957](#957)).
* Fixed AzureResourcePermissions.load to call Installation.load
([#962](#962)).
* Fixed installer script to reuse an existing UCX Cluster policy if
present ([#964](#964)).
* More `pylint` tuning
([#958](#958)).
* Refactor `workspace_client_mock` to have combine fixtures stored in
separate JSON files
([#955](#955)).

Dependency updates:

* Updated databricks-sdk requirement from ~=0.19.0 to ~=0.20.0
([#961](#961)).

dmoore247 pushed a commit that referenced this pull request


          Create storage credentials based on instance profiles and existing ro…

00b39d1

…les. (#869)

dmoore247 pushed a commit that referenced this pull request


          Release v0.13.1 (#966)

9977a8d

* Added secret detection logic to Azure service principal crawler
([#950](#950)).
* Create storage credentials based on instance profiles and existing
roles ([#869](#869)).
* Enforced `protected-access` pylint rule
([#956](#956)).
* Enforced `pylint` on unit and integration test code
([#953](#953)).
* Enforcing `invalid-name` pylint rule
([#957](#957)).
* Fixed AzureResourcePermissions.load to call Installation.load
([#962](#962)).
* Fixed installer script to reuse an existing UCX Cluster policy if
present ([#964](#964)).
* More `pylint` tuning
([#958](#958)).
* Refactor `workspace_client_mock` to have combine fixtures stored in
separate JSON files
([#955](#955)).

Dependency updates:

* Updated databricks-sdk requirement from ~=0.19.0 to ~=0.20.0
([#961](#961)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet