Skip to content

QoL: Friendly drop command 🐕‍🦺 #2720

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

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

anuunchin
Copy link
Contributor

@anuunchin anuunchin commented Jun 5, 2025

Description

This PR aims to make the drop cli command friendly with appropriate warnings.

  • Warn the user that they env vars or config files are used and inform that they should run the cli command from the correct location.
  • Warn the user that they are in the wrong repository when credentials can't be found. Currently the useful message is only given when a pipeline script is explicitly ran, but not when cli commands are used.
  • Warn the user about the tables that are to be dropped - with the location, name and any other relevant info. (I think this already partially implemented by @sh-rp 👀 )
  • When dlt-plus is installed, the drop command doesn't work with error DropCommand.__init__() got an unexpected keyword argument 'project'.

Related Issues

@anuunchin anuunchin self-assigned this Jun 5, 2025
Copy link

netlify bot commented Jun 5, 2025

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit a27187f
🔍 Latest deploy log https://app.netlify.com/projects/dlt-hub-docs/deploys/68481798c5e9a40008f8ad01

@anuunchin anuunchin force-pushed the fix/friendly-drop-command branch 5 times, most recently from 136a09f to 35f98e1 Compare June 10, 2025 09:17
@anuunchin anuunchin force-pushed the fix/friendly-drop-command branch from 35f98e1 to a27187f Compare June 10, 2025 11:31
import dlt

settings = run_context.active().settings_dir
abs_main_dir = dlt.current.pipeline().state["_local"]["initial_cwd"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is good but we can make it way better. initial_cwd is not 100% reliable and we have run_context. I'd do the following: whenever pipeline step is completed in Pipeline class we store run_context info into the local state (you have a method to set such state) ie.

{"last_run_context": "settings_dir": run_context.active().settings_dir, "local_dir": ... }

then here we can compare current settings dir and last run settings dir and show this warning if they are really different

best place place to do that is in extract/normalize/load/sync_destination just before function returns.
we do that here because we have a writable state at that point so we do not need to load/save it again

Copy link
Collaborator

Choose a reason for hiding this comment

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

dlt.current.pipeline() is not always available. if it is not - skip all the checks

Copy link
Collaborator

Choose a reason for hiding this comment

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

you have two pipeline commands to read/write local state:
get_local_state_val
set_local_state_val

main_path = main_module_file_path()
# check if entry point is from cli command
if main_path and main_path.endswith("dlt"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should run this check in any case

@@ -348,6 +349,16 @@ def _display_pending_packages() -> Tuple[Sequence[str], Sequence[str]]:
fmt.warning(warning)
return

fmt.echo(
Copy link
Collaborator

Choose a reason for hiding this comment

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

mhmmm maybe we should show this thing only when setting dirs differ? so exactly the same condition we have in config exeception?

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.

Make a drop command more user friendly
2 participants