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: force data load on import examples #24410

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Jun 15, 2023

SUMMARY

This attempts to fix some errors that we're seeing in current builds from pre 2.1 to master of ephemerals and docker tarball testing for releases. Some tables aren't loading into sqlite because the table is locked, so I'm passing in the force flag to reload the data if it was unsuccessful

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Sales_Dashboard

After:
Sales_Dashboard

TESTING INSTRUCTIONS

Ephemeral environments should succeed in loading example data

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@eschutho eschutho changed the title Elizabeth/fix examples load fix: retry import tables when not successful Jun 15, 2023
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #24410 (add5365) into master (19a9400) will increase coverage by 0.01%.
The diff coverage is 85.71%.

❗ Current head add5365 differs from pull request most recent head 8205609. Consider uploading reports for the commit 8205609 to get more accurate results

@@            Coverage Diff             @@
##           master   #24410      +/-   ##
==========================================
+ Coverage   68.95%   68.96%   +0.01%     
==========================================
  Files        1903     1904       +1     
  Lines       74070    74092      +22     
  Branches     8110     8120      +10     
==========================================
+ Hits        51077    51100      +23     
+ Misses      20881    20880       -1     
  Partials     2112     2112              
Flag Coverage Δ
hive 53.86% <0.00%> (ø)
mysql 79.26% <0.00%> (ø)
postgres 79.34% <0.00%> (ø)
presto 53.79% <0.00%> (ø)
python 83.31% <0.00%> (ø)
sqlite 77.83% <0.00%> (ø)
unit 54.56% <0.00%> (ø)

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

Impacted Files Coverage Δ
...acy-preset-chart-deckgl/src/components/Tooltip.tsx 0.00% <0.00%> (ø)
...lugins/plugin-chart-table/src/utils/formatValue.ts 66.66% <0.00%> (-3.93%) ⬇️
...c/components/Chart/DrillDetail/DrillDetailPane.tsx 75.32% <ø> (ø)
...-frontend/src/components/TableCollection/index.tsx 100.00% <ø> (ø)
...end/src/dashboard/components/SliceHeader/index.tsx 90.56% <ø> (ø)
...mponents/DataTablesPane/components/SamplesPane.tsx 97.67% <ø> (ø)
...ataTablesPane/components/SingleQueryResultPane.tsx 85.71% <ø> (ø)
superset/datasets/commands/importers/v1/utils.py 79.41% <0.00%> (ø)
...set-frontend/src/components/Table/VirtualTable.tsx 75.00% <75.00%> (-0.39%) ⬇️
...ponents/Chart/DrillDetail/DrillDetailMenuItems.tsx 93.87% <80.00%> (-1.68%) ⬇️
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@eschutho
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@eschutho Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@eschutho Ephemeral environment creation failed. Please check the Actions logs for details.

@eschutho eschutho force-pushed the elizabeth/fix-examples-load branch from 0539f2e to 6902e23 Compare June 15, 2023 19:34
@eschutho
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@eschutho Ephemeral environment spinning up at http://34.222.122.36:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@eschutho
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@eschutho Ephemeral environment spinning up at http://35.90.254.207:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@eschutho eschutho changed the title fix: retry import tables when not successful fix: force data load on import examples Jun 15, 2023
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

We might need to hit up github support to better understand WHY this works, but if it DOES work, then LGTM :)

@rusackas
Copy link
Member

rusackas commented Jun 16, 2023

Would you rather I merge or close #23893 ? I should probably just close it... I'll wait for this to merge though.

@eschutho
Copy link
Member Author

Would you rather I merge or close #23893 ? I should probably just close it... I'll wait for this to merge though.

Either way @rusackas. Thanks for that fix, I pulled it in here b/c it was the only way that I could test if this worked. :)

@eschutho eschutho merged commit b68de27 into apache:master Jun 16, 2023
@eschutho eschutho deleted the elizabeth/fix-examples-load branch June 16, 2023 19:55
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@sebastianliebscher
Copy link
Contributor

Hi @eschutho, while working on #25564 I noticed the following:

The check if: needs.config.outputs.has-secrets returns true in both cases when the variable has any content. So even if jobs have the check, they will run anyway.

The solution could be either to check if: needs.config.outputs.has-secrets == 1 or don't set has-secrets when there are no secrets.

@eschutho
Copy link
Member Author

Hi @eschutho, while working on #25564 I noticed the following:

The check if: needs.config.outputs.has-secrets returns true in both cases when the variable has any content. So even if jobs have the check, they will run anyway.

The solution could be either to check if: needs.config.outputs.has-secrets == 1 or don't set has-secrets when there are no secrets.

Hi, yeah I saw your comment in the other PR, too, thanks! This was a fix that @rusackas and I brought in to fix some CI failures, but we weren't sure the absolute need for it. If it works to remove the check like in #24464 then that seems to be a good fix, if you think that will work.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 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/S 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants