Skip to content
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 trampoline into newer CLI versions if found in $PATH #650

Merged
merged 12 commits into from
Jul 5, 2023

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Jun 16, 2023

No description provided.

@pietern
Copy link
Contributor Author

pietern commented Jun 16, 2023

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023

Codecov Report

Patch coverage has no change and project coverage change: -1.11 ⚠️

Comparison is base (112c887) 76.48% compared to head (a037d8e) 75.38%.

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     
Impacted Files Coverage Δ
databricks_cli/cli.py 0.00% <0.00%> (ø)
setup.py 0.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

return

# Try to trampoline only if we're running the CLI as 'databricks'.
if os.path.basename(sys.argv[0]) != 'databricks':

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to different between running as a full path and just command name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small nits/suggestions but LGTM

Comment on lines +104 to +110
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

databricks_cli/cli.py Outdated Show resolved Hide resolved
@pietern pietern merged commit 9bb7f5b into main Jul 5, 2023
@pietern pietern deleted the cli-trampoline-and-notice branch July 5, 2023 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants