Skip to content

Separate out snowflake source#836

Merged
xiaoyongzhu merged 79 commits intofeathr-ai:mainfrom
aabbasi-hbo:separate-snowflake-source
Nov 23, 2022
Merged

Separate out snowflake source#836
xiaoyongzhu merged 79 commits intofeathr-ai:mainfrom
aabbasi-hbo:separate-snowflake-source

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
xiaoyongzhu reviewed Nov 10, 2022
View reviewed changes
xiaoyongzhu
xiaoyongzhu reviewed Nov 10, 2022
View reviewed changes
@xiaoyongzhu
Copy link
Member

xiaoyongzhu commented Nov 10, 2022

@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

xiaoyongzhu commented Nov 10, 2022

@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 reviewed Nov 16, 2022
View reviewed changes
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

xiaoyongzhu commented Nov 23, 2022

@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

@xiaoyongzhu xiaoyongzhu xiaoyongzhu approved these changes

@hangfei hangfei hangfei 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