Skip to content

chore: add more required checks#12928

Merged
ktmud merged 2 commits into
apache:masterfrom
preset-io:elizabeth/more-checks
Feb 5, 2021
Merged

chore: add more required checks#12928
ktmud merged 2 commits into
apache:masterfrom
preset-io:elizabeth/more-checks

Conversation

@eschutho
Copy link
Copy Markdown
Member

@eschutho eschutho commented Feb 3, 2021

SUMMARY

Continuation of #12921, this PR adds more required github checks.

TEST PLAN

verify that the correct tests are required after this is merged. If this yml file doesn't match correctly, we will need to rename the checks so that they match or open an infra ticket to Apache to update it.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@eschutho eschutho changed the title add more required checks chore: add more required checks Feb 3, 2021
@eschutho
Copy link
Copy Markdown
Member Author

eschutho commented Feb 3, 2021

@ktmud @nytai

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 3, 2021

Codecov Report

Merging #12928 (59ca6d3) into master (b5b0c2c) will decrease coverage by 8.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12928      +/-   ##
==========================================
- Coverage   68.98%   60.85%   -8.13%     
==========================================
  Files        1025      966      -59     
  Lines       48765    45887    -2878     
  Branches     5188     4444     -744     
==========================================
- Hits        33639    27926    -5713     
- Misses      14992    17961    +2969     
+ Partials      134        0     -134     
Flag Coverage Δ
cypress 50.83% <ø> (ø)
javascript ?
python 66.87% <ø> (-0.48%) ⬇️

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

Impacted Files Coverage Δ
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/nativeFilters/ScopingTree.tsx 6.25% <0.00%> (-93.75%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx 11.76% <0.00%> (-88.24%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
superset-frontend/src/components/IconTooltip.tsx 13.33% <0.00%> (-86.67%) ⬇️
...rc/dashboard/components/gridComponents/Divider.jsx 13.33% <0.00%> (-86.67%) ⬇️
...perset-frontend/src/views/CRUD/welcome/Welcome.tsx 2.94% <0.00%> (-85.95%) ⬇️
... and 415 more

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 b5b0c2c...59ca6d3. Read the comment docs.

Comment thread .asf.yaml Outdated
# contexts are the names of checks that must pass
contexts:
- check
- lint (3.7)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think it's worth updating lint to python-lint as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, good idea. I removed it from this PR, added test-mysql instead. I'll rename lint in a separate pr

@ktmud
Copy link
Copy Markdown
Member

ktmud commented Feb 4, 2021

With the flexibility to set our own required checks, I think it's now time to get rid off the no-op "E2E / Cypress (chrome)" job that was obviously only meant to appease the original required checks.

@robdiciuccio correct me if I misunderstood the original intention of this construct.

@robdiciuccio
Copy link
Copy Markdown
Member

@ktmud yes, the E2E / Cypress job was purely to satisfy the required checks. Is the plan to remove e2e as a required check? If not, it may still be needed due to how the matrix operates.

@ktmud
Copy link
Copy Markdown
Member

ktmud commented Feb 4, 2021

@ktmud yes, the E2E / Cypress job was purely to satisfy the required checks. Is the plan to remove e2e as a required check? If not, it may still be needed due to how the matrix operates.

I think we do want to keep them as required checks. We can remove the no-op job and add required checks on cypress-matrix (1, chrome), cypress-matrix (2, chrome) and cypress-matrix (3, chrome) instead.

@eschutho
Copy link
Copy Markdown
Member Author

eschutho commented Feb 5, 2021

@ktmud do you think we should merge this on a Friday? I can set aside time to test, since this is the first time we're adding a check with the matrix.

@eschutho
Copy link
Copy Markdown
Member Author

eschutho commented Feb 5, 2021

🏷 ready-to-merge

@superset-github-bot superset-github-bot Bot added the need:merge The PR is ready to be merged label Feb 5, 2021
@ktmud ktmud merged commit 4708ed1 into apache:master Feb 5, 2021
@eschutho eschutho deleted the elizabeth/more-checks branch February 5, 2021 20:47
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
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 need:merge The PR is ready to be merged preset-io size/XS 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants