Add trampoline into newer CLI versions if found in $PATH #650
Conversation
|
The try/except body in |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #650 +/- ##
==========================================
- Coverage 76.48% 75.38% -1.11%
==========================================
Files 55 55
Lines 5048 5122 +74
==========================================
Hits 3861 3861
- Misses 1187 1261 +74
☔ View full report in Codecov by Sentry. |
| return | ||
|
|
||
| # Try to trampoline only if we're running the CLI as 'databricks'. | ||
| if os.path.basename(sys.argv[0]) != 'databricks': |
There was a problem hiding this comment.
Do we want to different between running as a full path and just command name?
There was a problem hiding this comment.
I'd like to but unfortunately, this is always set to the full path if it was resolved by looking at $PATH.
We can add an additional check to see if the path is absolute as well.
Then you can still call it as bin/databricks or something and it doesn't show the warning.
There was a problem hiding this comment.
TBH, for consistency it's probably fine to keep the check as is. If folks intend to run it they can use the environment variable to force it to and silence the error.
mgyucht
left a comment
There was a problem hiding this comment.
Some small nits/suggestions but LGTM
| # The new CLI is a single binary larger than 1MB. | ||
| # We use this as heuristic to tell if the new CLI is installed. | ||
| # Because this version of the CLI is much smaller, we do not | ||
| # need to dedup our own path to avoid an exec loop. | ||
| stat = os.stat(exec_path) | ||
| if stat.st_size < (1024 * 1024): | ||
| continue |
There was a problem hiding this comment.
I hope we don't break this...
If we're already loading the version of the CLI below, why don't we just call version anyways and get the result, breaking if we find something with version >= "0.100"?
There was a problem hiding this comment.
The version output in JSON only works on the new CLI, so if we remove this check we could be exec'ing into ourselves. The binary comes in at a decent 20MB so I think it's safe.
Are you thinking of another failure mode?
There was a problem hiding this comment.
Ah, I see... I was suggesting replacing this check with the version check. The other failure mode is if the python binary grows in size somehow.
No description provided.