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

fix(import_datasources): --sync flag works correctly #18046

Merged

Conversation

cccs-Dustin
Copy link
Contributor

SUMMARY

  1. Modified line 573 in /superset/superset/cli.py, as the previous code was not properly assigning a boolean value to sync_columns and sync_metrics.
  2. Added lines 324-413 in superset/tests/integration_tests/cli_tests.py for 3 new unit tests to make sure the --sync flag is working correctly.

By modifying the code in /superset/superset/cli.py, the Superset import_datasources cli now correctly functions with the --sync flag.

TESTING INSTRUCTIONS

  1. Add an existing yaml file to superset by using the Superset cli:
    superset import-datasources -p ~/datasets/data.yaml
  2. Once the yaml file has been added, try using the command again but with the "-s"/"--sync" flag (imitating what it would be like if you modified a column name or metric in the YAML file and wanted it to be mirrored in the Superset Dataset):
    superset import-datasources -s "metrics,columns" -p ~/datasets/data.yaml
  3. Now that the "-s"/"--sync" flag works as intended, it deletes metrics and/or columns in the DB that are not specified in the YAML file (when using the "-s"/"--sync" flag).

ADDITIONAL INFORMATION

…s cli command. Also added unit tests for all of the edge cases (with both metrics & columns, with just columns, and with just metrics)
…Removing it might cause headaches for someone trying to work on this particular piece of code in the future
@cccs-Dustin cccs-Dustin changed the title [WIP] fix(import_datasources): --sync flag works correctly fix(import_datasources): --sync flag works correctly Jan 14, 2022
@cccs-Dustin
Copy link
Contributor Author

@geido Are you able to approve running workflows for me as I am a first-time contributor?

@geido
Copy link
Member

geido commented Jan 19, 2022

@geido Are you able to approve running workflows for me as I am a first-time contributor?

Sure thing

@cccs-Dustin cccs-Dustin changed the title fix(import_datasources): --sync flag works correctly [WIP] fix(import_datasources): --sync flag works correctly Jan 19, 2022
…om:CybercentreCanada/superset into fix/superset-cli-import-datasources-s-flag

Updated the cli_tests.py file as the merge that was completed changed the format of the unit test. I needed to update my unit tests to the new format
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 24, 2022
@cccs-Dustin cccs-Dustin changed the title [WIP] fix(import_datasources): --sync flag works correctly fix(import_datasources): --sync flag works correctly Jan 24, 2022
@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #18046 (9061bf2) into master (e632193) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9061bf2 differs from pull request most recent head bfbb70f. Consider uploading reports for the commit bfbb70f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #18046   +/-   ##
=======================================
  Coverage   65.94%   65.95%           
=======================================
  Files        1584     1584           
  Lines       62055    62055           
  Branches     6273     6273           
=======================================
+ Hits        40925    40926    +1     
+ Misses      19509    19508    -1     
  Partials     1621     1621           
Flag Coverage Δ
hive 52.15% <0.00%> (ø)
mysql ?
postgres 81.27% <100.00%> (+0.04%) ⬆️
presto 51.99% <0.00%> (ø)
python 81.67% <100.00%> (+<0.01%) ⬆️
sqlite 80.96% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/cli/importexport.py 80.00% <100.00%> (+7.56%) ⬆️
superset/common/utils/dataframe_utils.py 85.71% <0.00%> (-7.15%) ⬇️
superset/db_engine_specs/mysql.py 93.97% <0.00%> (-3.62%) ⬇️
superset/models/core.py 88.32% <0.00%> (-0.73%) ⬇️
superset/views/core.py 77.29% <0.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e632193...bfbb70f. Read the comment docs.

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @cccs-Dustin!

@geido geido merged commit 2dd64f9 into apache:master Jan 25, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* Added code that properly accepts the -s flag on the import-datasources cli command. Also added unit tests for all of the edge cases (with both metrics & columns, with just columns, and with just metrics)

* Files were reformated using the 'pre-commit run --all-files' command

* added '*args: Any' back into v0.py as it did not need to be removed. Removing it might cause headaches for someone trying to work on this particular piece of code in the future

* Fixed the merge conflict as the cli.py was moved to another directory

* Modified my created unit tests to work with the new format of uni tests since the merge

* Modified my created unit tests to work with the new format of uni tests since the merge

* Fixed errors which were encountered while using the unit tests
ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
* Added code that properly accepts the -s flag on the import-datasources cli command. Also added unit tests for all of the edge cases (with both metrics & columns, with just columns, and with just metrics)

* Files were reformated using the 'pre-commit run --all-files' command

* added '*args: Any' back into v0.py as it did not need to be removed. Removing it might cause headaches for someone trying to work on this particular piece of code in the future

* Fixed the merge conflict as the cli.py was moved to another directory

* Modified my created unit tests to work with the new format of uni tests since the merge

* Modified my created unit tests to work with the new format of uni tests since the merge

* Fixed errors which were encountered while using the unit tests
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* Added code that properly accepts the -s flag on the import-datasources cli command. Also added unit tests for all of the edge cases (with both metrics & columns, with just columns, and with just metrics)

* Files were reformated using the 'pre-commit run --all-files' command

* added '*args: Any' back into v0.py as it did not need to be removed. Removing it might cause headaches for someone trying to work on this particular piece of code in the future

* Fixed the merge conflict as the cli.py was moved to another directory

* Modified my created unit tests to work with the new format of uni tests since the merge

* Modified my created unit tests to work with the new format of uni tests since the merge

* Fixed errors which were encountered while using the unit tests
@cccs-Dustin cccs-Dustin deleted the fix/superset-cli-import-datasources-s-flag branch March 15, 2022 12:14
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "-s"/"--sync" flags for the Superset import-datasources cli command do not function properly
4 participants