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

I want superset {im, ex}port to *fail* if unsuccessful #16956

Closed
3 tasks done
EBoisseauSierra opened this issue Oct 4, 2021 · 4 comments
Closed
3 tasks done

I want superset {im, ex}port to *fail* if unsuccessful #16956

EBoisseauSierra opened this issue Oct 4, 2021 · 4 comments
Labels
#bug Bug report

Comments

@EBoisseauSierra
Copy link
Contributor

When the import of a dashboard export fails, the logger lists an exception but the Python script exits as successful. This is a bit counter-intuitive as the import/export has actually failed.

How to reproduce the bug

  1. Import a non-working export ZIP archive,
  2. The exception is caught by that broad except Exception catch.
  3. The Python script exits with a 0 exit code.

Expected results

Because the command has effectively failed to execute, I expect the Python script to exits with a non-0 exit code.

Actual results

0-exit code.

Environment

  • superset version: 1.3.0
  • python version: 3.8.12
  • any feature flags active: "VERSION_EXPORT": True

Checklist

Make sure to follow these steps before submitting your issue - thank you!

  • I have checked the superset logs for python stacktraces and included it here as text if there are any.
  • I have reproduced the issue with at least the latest released version of superset.
  • I have checked the issue tracker for the same issue and I haven't found one similar.
@EBoisseauSierra EBoisseauSierra added the #bug Bug report label Oct 4, 2021
@EBoisseauSierra
Copy link
Contributor Author

I understand this behaviour is intentional (this happens multiple times, plus # pylint: disable=broad-except)… but I'd like to discuss whether a failing script wouldn't be more appropriate.

This would surely make my life writing a Puppet manifest easier!

@dpgaspar
Copy link
Member

dpgaspar commented Oct 4, 2021

Hi @EBoisseauSierra I agree, are you willing to open a fix PR?
feel free to ping for any help, if needed

@EBoisseauSierra
Copy link
Contributor Author

of course (: ! I should draft something by the end of the week and I'll gladly ping you once the WIP PR is out.

EBoisseauSierra added a commit to EBoisseauSierra/superset that referenced this issue Oct 19, 2021
Bubble exception up when failing import or export

During a CLI import or export of dashboards, if the process fails, the
exception it caught and a simple message is sent to the logger.
This makes that from a shell point of view, the script was successfull —
cf. apache#16956.

To prevent this, we want to ensure that the process exits with an error
(i.e., a non-0 exit-code) should the export or import fail mid-flight.

Signed-off-by: Étienne Boisseau-Sierra <[email protected]>
dpgaspar pushed a commit that referenced this issue Oct 29, 2021
* Test that failing export or import is done properly

For each CLI entry-point we will modify, we make sure that:

- a failing process exits with a non-0 exit code,
- an error is logged.

Signed-off-by: Étienne Boisseau-Sierra <[email protected]>

* Exit process with error if export/import failed

Bubble exception up when failing import or export

During a CLI import or export of dashboards, if the process fails, the
exception it caught and a simple message is sent to the logger.
This makes that from a shell point of view, the script was successfull —
cf. #16956.

To prevent this, we want to ensure that the process exits with an error
(i.e., a non-0 exit-code) should the export or import fail mid-flight.

Signed-off-by: Étienne Boisseau-Sierra <[email protected]>
@EBoisseauSierra
Copy link
Contributor Author

Closing as fixed by #16976

eschutho pushed a commit that referenced this issue Nov 17, 2021
* Test that failing export or import is done properly

For each CLI entry-point we will modify, we make sure that:

- a failing process exits with a non-0 exit code,
- an error is logged.

Signed-off-by: Étienne Boisseau-Sierra <[email protected]>

* Exit process with error if export/import failed

Bubble exception up when failing import or export

During a CLI import or export of dashboards, if the process fails, the
exception it caught and a simple message is sent to the logger.
This makes that from a shell point of view, the script was successfull —
cf. #16956.

To prevent this, we want to ensure that the process exits with an error
(i.e., a non-0 exit-code) should the export or import fail mid-flight.

Signed-off-by: Étienne Boisseau-Sierra <[email protected]>
(cherry picked from commit f0c0ef7)
AAfghahi pushed a commit that referenced this issue Jan 10, 2022
* Test that failing export or import is done properly

For each CLI entry-point we will modify, we make sure that:

- a failing process exits with a non-0 exit code,
- an error is logged.

Signed-off-by: Étienne Boisseau-Sierra <[email protected]>

* Exit process with error if export/import failed

Bubble exception up when failing import or export

During a CLI import or export of dashboards, if the process fails, the
exception it caught and a simple message is sent to the logger.
This makes that from a shell point of view, the script was successfull —
cf. #16956.

To prevent this, we want to ensure that the process exits with an error
(i.e., a non-0 exit-code) should the export or import fail mid-flight.

Signed-off-by: Étienne Boisseau-Sierra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#bug Bug report
Projects
None yet
Development

No branches or pull requests

2 participants