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

Move custom resources to Dataset Environment extension #647

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Aug 9, 2023

Feature or Bugfix

  • Refactoring

Detail

  • moved dataset related custom resources to Dataset module
  • created EnvironmentExtension for the custom resources
  • fix one small bug in creation of datasets - the KMS alias name defaulted to sse-s3 for created datasets, while it should be the bucketName

Relates

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlpzx dlpzx marked this pull request as ready for review August 10, 2023 08:59
@dlpzx dlpzx requested a review from nikpodsh August 10, 2023 09:00
# Set PivotRole as Lake Formation data lake admin
entry_point = str(
pathlib.PosixPath(os.path.dirname(__file__),
'./assets/lakeformationdefaultsettings').resolve()
Copy link
Contributor

Choose a reason for hiding this comment

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

In DatasetGlueProfilerExtension, the relative path is different. We need to check which one is correct

Copy link
Contributor Author

@dlpzx dlpzx Aug 11, 2023

Choose a reason for hiding this comment

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

I tested the creation of the Dataset with the extensions and it worked fine with this paths. At first I had the same level for both but i think because of the libraries we use they are defined differently

@nikpodsh nikpodsh added this to the v2.0.0 milestone Aug 11, 2023
@nikpodsh nikpodsh added the type: modularization Code refactoring project label Aug 11, 2023
@nikpodsh
Copy link
Contributor

I have only one question. The rest changes look good to me!

@dlpzx dlpzx merged commit 2933f18 into data-dot-all:modularization-main Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: modularization Code refactoring project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants