-
Notifications
You must be signed in to change notification settings - Fork 996
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
Add current directory in sys path for CLI commands that might depend on custom providers #1594
Add current directory in sys path for CLI commands that might depend on custom providers #1594
Conversation
Hi @MattDelac. Thanks for your PR. I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
8da951a
to
80539d7
Compare
Codecov Report
@@ Coverage Diff @@
## master #1594 +/- ##
==========================================
- Coverage 83.59% 83.55% -0.04%
==========================================
Files 65 65
Lines 5755 5760 +5
==========================================
+ Hits 4811 4813 +2
- Misses 944 947 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
80539d7
to
bcd0e65
Compare
/ok-to-test |
/kind bug |
@MattDelac btw there is a fix for the flaky test over here #1593 |
@MattDelac I think it may be possible to just use https://github.com/feast-dev/feast/pull/1594/files#diff-cc33a5e7afa90467a37c11f60b2a380bd86af97524300ca6484a0373e2f00cb6R70 so that we don't need to append in multiple places. Also, I think we need to support remote repos using the |
bcd0e65
to
35917dd
Compare
Yes I put it in
Sounds like out of topic and related to #1556 no ? Unsure I understand your point here |
35917dd
to
201d676
Compare
Signed-off-by: Matt Delacour <matt.delacour@shopify.com>
201d676
to
76bff49
Compare
@woop This PR is ready for a final review |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MattDelac, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Signed-off-by: Matt Delacour <matt.delacour@shopify.com>
What this PR does / why we need it:
Only
feast apply
works with custom providers. Other CLI commands that depend on the provider don't work with custom ones.This is because the current directory is append to the SYS PATH only during
feast apply
Which issue(s) this PR fixes:
Fixes #1589
Does this PR introduce a user-facing change?: