Skip to content

Support 'enabled' configurations for offline stores#545

Merged
blrchen merged 7 commits intofeathr-ai:mainfrom
enya-yx:enya/enable-flags
Aug 24, 2022
Merged

Support 'enabled' configurations for offline stores#545
blrchen merged 7 commits intofeathr-ai:mainfrom
enya-yx:enya/enable-flags

Conversation

@enya-yx
Copy link
Collaborator

@enya-yx enya-yx commented Aug 2, 2022

For offline stores, S3/ADLS/WASB/JDBC/Snowflake, if any of its 'enabled' config variables are set to 'false' or undefined their other related environment variables will not be visited.

@xiaoyongzhu xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Aug 2, 2022
@xiaoyongzhu
Copy link
Member

xiaoyongzhu commented Aug 2, 2022

Quick comment - instead just return empty string, we should also eliminate the parameter names below:

                '--s3-config', self._get_s3_config_str(),
                '--adls-config', self._get_adls_config_str(),
                '--blob-config', self._get_blob_config_str(),
                '--sql-config', self._get_sql_config_str(),
                '--snowflake-config', self._get_snowflake_config_str()

i.e. if s3 is not configured, it doesn't make sense to have --s3-config in the param list

@enya-yx
Copy link
Collaborator Author

enya-yx commented Aug 22, 2022

Quick comment - instead just return empty string, we should also eliminate the parameter names below:

                '--s3-config', self._get_s3_config_str(),
                '--adls-config', self._get_adls_config_str(),
                '--blob-config', self._get_blob_config_str(),
                '--sql-config', self._get_sql_config_str(),
                '--snowflake-config', self._get_snowflake_config_str()

i.e. if s3 is not configured, it doesn't make sense to have --s3-config in the param list

Modified the solution in new commit.

@jainr jainr self-requested a review August 23, 2022 22:25
jainr
jainr previously approved these changes Aug 23, 2022
View reviewed changes
Copy link
Collaborator

@jainr jainr 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 update, changes look good to me.

@blrchen blrchen merged commit 31414fe into feathr-ai:main Aug 24, 2022
ahlag pushed a commit to ahlag/feathr that referenced this pull request Aug 26, 2022
@enya-yx @ahlag
Support 'enabled' configurations for offline stores (feathr-ai#545)
0008290 
For offline stores, S3/ADLS/WASB/JDBC/Snowflake, if any of its 'enabled' config variables are set to 'false' or undefined their other related environment variables will not be visited.
@enya-yx enya-yx mentioned this pull request Sep 21, 2022
Open
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

@blrchen blrchen blrchen approved these changes

@jainr jainr jainr left review comments

@windoze windoze Awaiting requested review from windoze

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

@hangfei hangfei Awaiting requested review from hangfei

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.

4 participants

@enya-yx @xiaoyongzhu @blrchen @jainr