feat: allow path resolver to look at additional paths #78
Conversation
- search for executables on additional paths that are not on `$PATH` - additional paths take precedence before searching for executables on `$PATH` - rev protocol version to 0.2 with introduction of additional search paths.
|
Are there any integration tests we can add to make sure we don't break this behavior in the future? |
- address comments - add functional tests
|
@sanathkr we are definitely revving the version. That was an old comment, disregard that. |
- extra func test for back compat
There was a problem hiding this comment.
Looks good to me! Thanks for the changes and functional test. There was a ton of discussion in this PR to clarify what is the version check, why we need it, and how it works. We should document this in a design document for sure. Otherwise this will be lost as tribal knowledge.
Will you be able to add a design document summarizing this discussion? This document should provide mechanics on how this work and provide guidance on when/when-not to rev the version. I am okay if you want to add design doc in a separate PR.
| @@ -50,17 +50,19 @@ def tearDown(self): | |||
| shutil.rmtree(self.artifacts_dir) | |||
|
|
|||
| @parameterized.expand([ | |||
| ("request_through_stdin"), | |||
| ("request_through_argument") | |||
| ("request_through_stdin", lambda_builders_protocol_version), | |||
There was a problem hiding this comment.
can you add a test that fails compat check?
There was a problem hiding this comment.
yes the test is present below.
There was a problem hiding this comment.
| @@ -23,7 +23,7 @@ install: | |||
| - "%PYTHON%\\python.exe -m pip install -e ." | |||
| - "set PATH=C:\\Ruby25-x64\\bin;%PATH%" | |||
| - "gem --version" | |||
| - "gem install bundler -v 1.17.3 --no-ri --no-rdoc" | |||
| - "gem install bundler -v 1.17.3" | |||
There was a problem hiding this comment.
yeah, this is some recent breaking change with gem
| ("request_through_stdin"), | ||
| ("request_through_argument") | ||
| ]) | ||
| def test_run_hello_workflow_incompatible(self, flavor): |
There was a problem hiding this comment.
This test is good for now, but it will need an upgrade when we change the version check logic or bump beyond 2.0.
$PATHon
$PATHpaths.
Issue #, if available:
#77
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.