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.

Sign up for GitHub

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

Jump to bottom

fix(import-datasources): Use "admin" user as default for importing datasources #27154

Merged
merged 7 commits into from
Feb 27, 2024
Merged

fix(import-datasources): Use "admin" user as default for importing datasources #27154

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
468e952
Use "admin" user as default for importing datasources
ddxv Feb 19, 2024
ef1e28d
Merge branch 'master' into import-datasources-bugfix
ddxv Feb 20, 2024
893ccd1
Merge remote-tracking branch 'upstream/master' into import-datasource…
ddxv Feb 20, 2024
0cfa81c
Try with override_user
ddxv Feb 20, 2024
ce0b3bd
Set username as optional input for import_datasources with default of…
ddxv Feb 26, 2024
4993006
Set default and required=false in click.option
ddxv Feb 26, 2024
c38f17a
Fix typo in option help
ddxv Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions superset/cli/importexport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

from superset import security_manager
from superset.extensions import db
from superset.utils.core import override_user

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -176,20 +177,21 @@
from superset.commands.dataset.importers.dispatcher import ImportDatasetsCommand
from superset.commands.importers.v1.utils import get_contents_from_bundle

if is_zipfile(path):
with ZipFile(path) as bundle:
contents = get_contents_from_bundle(bundle)
else:
with open(path) as file:
contents = {path: file.read()}
try:
ImportDatasetsCommand(contents, overwrite=True).run()
except Exception: # pylint: disable=broad-except
logger.exception(
"There was an error when importing the dataset(s), please check the "
"exception traceback in the log"
)
sys.exit(1)
with override_user(user=security_manager.find_user(username="admin")):
Copy link
Member

Choose a reason for hiding this comment

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

it seems it would probably make sense to add a username option here and leave admin as the default. @betodealmeida what to you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a consistency point of view, this makes sense to me. Let me know if you'd like me to go ahead and try with --username as an optional option.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with Daniel's feedback there

Copy link
Member

Choose a reason for hiding this comment

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

Hey @ddxv wondering if you had a chance to look at this last change. So tempted to merge it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geido Are you referring to Daniel's suggestion? I'm all for that. Do we need to wait for any confirmation from @betodealmeida? Just to be sure, I didn't see any recent "change" to code though, so let me know if I am missing something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geido I went ahead and pushed the change suggested by Daniel for an optional field with default="admin". My only thought on that is that if you want to match import_dashboards it would be a required field without a default. This might be better just to have their API be the same for documentation and use, but let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

@ddxv it would be great if you want to open a PR to match import_dashboards unless there is any opposition. For the time being I am approving this PR and moving forward.

if is_zipfile(path):
with ZipFile(path) as bundle:
contents = get_contents_from_bundle(bundle)

Check warning on line 183 in superset/cli/importexport.py

View check run for this annotation

Codecov / codecov/patch

superset/cli/importexport.py#L180-L183 

Added lines #L180 - L183 were not covered by tests
else:
with open(path) as file:
contents = {path: file.read()}
try:
ImportDatasetsCommand(contents, overwrite=True).run()
except Exception: # pylint: disable=broad-except
logger.exception(

Check warning on line 190 in superset/cli/importexport.py

View check run for this annotation

Codecov / codecov/patch

superset/cli/importexport.py#L185-L190 

Added lines #L185 - L190 were not covered by tests
"There was an error when importing the dataset(s), please check the "
"exception traceback in the log"
)
sys.exit(1)

Check warning on line 194 in superset/cli/importexport.py

View check run for this annotation

Codecov / codecov/patch

superset/cli/importexport.py#L194 

Added line #L194 was not covered by tests


@click.command()
Expand Down
Loading