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.

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

Separate out snowflake source #836

Merged
merged 79 commits into from
Nov 23, 2022
Merged

Separate out snowflake source #836

merged 79 commits into from
Nov 23, 2022

Conversation

aabbasi-hbo
Copy link
Collaborator

@aabbasi-hbo aabbasi-hbo commented Nov 6, 2022 •
edited
Loading

Description

Separates Snowflake implementation into its own functionality. Fix to allow for registry authentication when using AWS

Resolves #835

How was this PR tested?

Updated/Added Unit Tests

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to clarify your proposed changes.

Allows users to directly use SnowflakeSource instead of specifying snowflake information in HdfsSource

xiaoyongzhu and others added 30 commits September 25, 2022 00:27
@xiaoyongzhu
Add more secret manager support
c31906c
@xiaoyongzhu
Add abstract class
e96459a
@xiaoyongzhu
Update feathr-configuration-and-env.md
2d6c135
@xiaoyongzhu
Update _envvariableutil.py
f616522
@xiaoyongzhu
add tests for aws secrets manager
cdcd612
@xiaoyongzhu
Update test_secrets_read.py
aa5fdda
@xiaoyongzhu
fix tests
a6870d9
@xiaoyongzhu
Update test_secrets_read.py
997a2b1
@xiaoyongzhu
fix test
8be6a42
@xiaoyongzhu
Update pull_request_push_test.yml
e617b99
@xiaoyongzhu
Merge branch 'main' into secret_manager
435e24f
@aabbasi-hbo
get_secrets_update
9cb332c
@aabbasi-hbo
move import statement
218123f
@aabbasi-hbo
update spelling
07a8cf0
@aabbasi-hbo
update raise exception
44a3ce0
@aabbasi-hbo
revert
87cd083
@aabbasi-hbo
feature registry hack
bd3d2b0
@aabbasi-hbo
query for uppercase
0531788
@aabbasi-hbo
add snowflake source
57ee474
@aabbasi-hbo
remove snowflake type
a8e566b
@aabbasi-hbo
enableDebugLogger
268f600
@aabbasi-hbo
add logging
1030c3e
@aabbasi-hbo
simple path snowflake fix
6590287
@aabbasi-hbo
snowflake-update
9230aab
@aabbasi-hbo
fix bugs/log
46db910
@aabbasi-hbo
get_snowflake_path
6fa8596
@aabbasi-hbo
update get_snowflake_path
b71b415
@aabbasi-hbo
Merge pull request #2 from feathr-ai/main
573a2d6 
update local repo main
@aabbasi-hbo
Merge branch 'snowflake-source-scala-update' into main
5d724f1
@aabbasi-hbo
Merge pull request #3 from aabbasi-hbo/main
0668b51 
merge main to feature
@xiaoyongzhu
Copy link
Member

@aabbasi-hbo Thanks for the contribution! This is really a good PR, clean and to the point.

The only comment I have is - can you separate the registry part? Since that is causing some failures so it might be better to separate those.

@xiaoyongzhu
Copy link
Member

@aabbasi-hbo Thanks for the contribution! This is really a good PR, clean and to the point.

The only comment I have is - can you separate the registry part? Since that is causing some failures so it might be better to separate those.

Other parts looks really great BTW. Thanks for the contribution!

xiaoyongzhu
xiaoyongzhu previously approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@xiaoyongzhu xiaoyongzhu 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 the contribution, this LGTM. Please also help resolve the conflicts.

hangfei
hangfei previously approved these changes Nov 18, 2022
View reviewed changes
aabbasi-hbo
aabbasi-hbo commented Nov 23, 2022
View reviewed changes
Copy link
Collaborator Author

@aabbasi-hbo aabbasi-hbo left a comment

Choose a reason for hiding this comment

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

@xiaoyongzhu @hangfei any more changes/fixes needed for this PR?

@xiaoyongzhu
Copy link
Member

@aabbasi-hbo thanks for the contribution! I don't have more comments. I'll circle back to @hangfei and let him approve as well.

hangfei
hangfei approved these changes Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@hangfei hangfei left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you!

@xiaoyongzhu xiaoyongzhu merged commit c21d89d into feathr-ai:main Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangfei hangfei hangfei approved these changes

@xiaoyongzhu xiaoyongzhu xiaoyongzhu approved these changes

@jaymo001 jaymo001 Awaiting requested review from jaymo001

@blrchen blrchen Awaiting requested review from blrchen

@enya-yx enya-yx Awaiting requested review from enya-yx

@jainr jainr Awaiting requested review from jainr

@Yuqing-cat Yuqing-cat Awaiting requested review from Yuqing-cat

Assignees
No one assigned
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] Separate out Snowflake Source
3 participants
@aabbasi-hbo @xiaoyongzhu @hangfei