chore: add required review on master #12694

eschutho · 2021-01-22T18:57:14Z

SUMMARY

This change requires at least one approving review before committers can merge PRs to master. It should not (in theory) affect mergeability with regard to passing tests.

https://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features

ADDITIONAL INFORMATION

eschutho · 2021-01-22T18:58:32Z

@mistercrunch @robdiciuccio @nytai

ktmud · 2021-01-22T19:28:58Z

Should we try adding "Required" checks in asf.yml as well? Just to be explicit.

codecov-io · 2021-01-22T19:37:11Z

Codecov Report

Merging #12694 (97b93c2) into master (c85b4c7) will decrease coverage by 7.79%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12694      +/-   ##
==========================================
- Coverage   66.81%   59.01%   -7.80%     
==========================================
  Files        1017      965      -52     
  Lines       49734    48099    -1635     
  Branches     4864     4421     -443     
==========================================
- Hits        33229    28386    -4843     
- Misses      16382    19713    +3331     
+ Partials      123        0     -123

Flag	Coverage Δ
cypress	`50.88% <ø> (-0.12%)`	⬇️
javascript	`?`
python	`63.51% <ø> (-0.49%)`	⬇️

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%)`	⬇️
.../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%)`	⬇️
...end/src/SqlLab/components/ExploreResultsButton.jsx	`8.00% <0.00%> (-84.00%)`	⬇️
...nd/src/views/CRUD/data/query/QueryPreviewModal.tsx	`14.70% <0.00%> (-82.97%)`	⬇️
... and 433 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 c85b4c7...97b93c2. Read the comment docs.

eschutho · 2021-01-22T20:57:14Z

Should we try adding "Required" checks in asf.yml as well? Just to be explicit.

yeah, thanks @ktmud. I'm looking at this note in the docs, too: Enabling any of the above checks overrides what may have been set previously, so you'll need to add all the existing checks to your .asf.yaml to reproduce those previously set manually by Infra. I'll add in the entire section just to be sure that nothing is overwritten.

eschutho · 2021-01-22T21:50:19Z

.asf.yaml

+      required_pull_request_reviews:
+        dismiss_stale_reviews: true
+        require_code_owner_reviews: true
+        required_approving_review_count: 1


definitely would like a second pair of eyes on all this. I don't think there's a way to test before merging.

The only risk I see is the contexts are not correct. Let's just be ready to fix/revert if needed

Thanks, yeah, I set up: #12698 and will rebase when this lands. We might as well wait until next week, too, to land this.

nytai · 2021-01-22T21:58:54Z

.asf.yaml

+          - test-sqlite
+
+      required_pull_request_reviews:
+        dismiss_stale_reviews: true


Do we want this? It might cause some pain, say if a PR is approved but a rebase is needed before merging.

good call. updated.

ktmud · 2021-01-22T22:22:53Z

.asf.yaml

+          - lint
+          - test-mysql
+          - test-postgres
+          - test-sqlite


It's a little unclear which names should the contexts use.

This API returns only the job name, with Matrix suffix:

curl -s \ -H "Authorization: token ${GITHUB_TOKEN}" \ https://api.github.com/repos/apache/superset/commits/master/check-runs | jq ".check_runs[] | .name" "test-sqlite (3.7)" "pre-commit (3.7)" "Prefer Typescript" "test-postgres-presto (3.8)" "test-postgres (3.8)" "babel-extract (3.7)" "test-postgres (3.7)" "lint (3.7)" "build" "License Check" "build" "test-postgres-hive (3.8)" "test-postgres-hive (3.7)" "frontend-check" "test-mysql (3.7)" "build" "babel-extract (3.7)"

@ktmud do you have these actions set up in your fork? If so, can you check the name that appears under the settings tab for the repo?

I'm assuming the API will use the same names as displayed here?

We might want to rename a couple of our jobs to avoid confusion (there are a couple of duplicate "build"s).

I guess the safest route would be to have a very limited list here to override the admin settings, then add them back in a separate PR?

I think those are the names we need to use, though they do look quite different than the names reported on PRs. I don't think the risk here is that high, I doubt committers will be merging PRs with failing status checks (even if they're not actually marked required). We could make and announcement just in case, "You might see some changes to the required checks as we figure things out, please don't merge anything that's failing a check"

The docs show them as <Action Name>/<Job Name>:

contexts: - gh-infra/jenkins - another/build-that-must-pass

although I realize that the action name was missing in some of the tests when I pulled them in, so I can update that. Looks to me that we would have to provide the action name in order for the duplicated job names to even work.

I think the risk here is that if you add checks not matching the actual context names as required, everyone will (may) be blocked from merging until INFRA intervenes.

Right, that's a good point. We should probably do this during hours of low activity. Worst case, we revert quickly and none of the check will be required. Without the admin repo privileges we are kind of shooting in the dark though

Following on this scenario, do you think all the "blocked" prs would automatically be mergable after we revert this? Do you think there's any risk that we won't be able to unblock them? I think worst case scenario, we should be able to close and open the pr to retrigger the checks, which would just be time consuming.

I think rather than revert we'd want to set the context as empty or remove the ones that end up stuck

eschutho · 2021-01-26T00:41:25Z

.asf.yaml

+          - Python Misc/lint
+          - Python MySQL/test-mysql
+          - Python MySQL/test-postgres
+          - Python MySQL/test-sqlite


@nytai @ktmud Do we feel comfortable with this list or do we want to use the api response instead?

I found a few other projects using this feature
https://github.com/apache/skywalking-eyes/blob/main/.asf.yaml
https://github.com/apache/skywalking/blob/master/.asf.yaml
https://github.com/apache/skywalking-python/blob/master/.asf.yaml

It seems they're using the portion after the /, which would result in duplicates for us. I think this looks good to me

I suspect if a match isn't found in the list of recent contexts nothing happens

I suspect it will wait for that check forever... Maybe let's add a very short list to override the admin settings first, then gradually add them back?

When you go to settings you can't just enter any string, you have to choose from the list of "status checks found in the last week for this repository". I think it needs to be a context the repo has seen before. Also, here they have build set but no check is marked as required.
https://github.com/apache/skywalking-client-js/blob/04dd6b3b024f838b3b92002a370ce3b7f70638b4/.asf.yaml#L38

Although, it may be a good idea to be extra safe here as we may end up in a stuck state waiting for apache infra.

Tested with a personal fork, now I'm 90% sure GitHub will just be forever waiting for these unidentifiable checks:

IMO the safest course of actions is:

Update this PR to include only the simplest check (e.g. "check" for "PR Lint") in the required list, just to override manual admin settings.

Once the CI passes, merge it.

Make another PR to rename the jobs with ambiguous names

Maybe another PR to add other required checks back to the "required" list. Could combine with 3, too.

ah, this is great. @ktmud it looks like three of four of those context strings didn't match any existing checks. Did "build" work and match to Frontend / build?

Actually based on your other screenshot, I think I should go with the whole string with the space PR Lint / check for example.

Thanks for the thorough investigation @ktmud. I think you're right, this file is just used to make a request to the github api.

eschutho · 2021-01-27T00:15:24Z

.asf.yaml

-          - Python MySQL/test-mysql
-          - Python MySQL/test-postgres
-          - Python MySQL/test-sqlite
+          - PR Lint / check


based on how @ktmud's example, it looks like this is the proper format, and removed the others for now if we wanted to just test one.

Sorry, I think you need to remove the "PR Lint / " prefix, too (i.e. just use "check"). The contexts config will only match the job name, not the workflow name.

See here the checks for Python MySQL / test-mysql (3.7) here: NUDA5020#1

This is what I sent to the API:

{ "required_status_checks": { "strict": true, "contexts": [ "build", "lint", "PR Lint / check", "check", "Python MySQL/test-mysql", "Python MySQL/test-mysql (3.7)", "abc/def" ] } }

eschutho · 2021-01-27T00:16:06Z

we could also if we wanted to mitigate risk, just push out the required reviewer change in this pr and then add the required context in a separate pr, too.

eschutho · 2021-01-27T20:55:42Z

Ok, this is updated to only require "check". The test on a different branch didn't work. So the plan then is to land this, which should only make PR Lint/ check required. Then if that works, we add build, lint, test-mysql, test-postgres, test-sqlite and Cypress and we're not sure what is going to happen with the duplicate build job.

ktmud · 2021-01-28T00:27:24Z

we're not sure what is going to happen with the duplicate build job.

I think it will match both jobs. And the CI will not be green unless both jobs pass. We should probably rename them before adding them back to required.

eschutho · 2021-01-28T22:20:59Z

Rerunning checks

nytai · 2021-02-03T01:27:40Z

That worked! Looks like "PR Lint / check" is the only required check now

ktmud · 2021-02-03T01:31:48Z

Yeah! Was just gonna report the same thing:

* master: (23 commits) feat(explore): clear search on dataset change (apache#12909) chore: remove SIP-38 feature flag (apache#12894) fix: Config for dataset health check (apache#12906) fix(chart): allow null for most query object props (apache#12905) feat: add separate endpoint to fetch function names for autocomplete (apache#12840) chore: add required review on master (apache#12694) fix: comment typo (apache#12898) Migrates Radio component from Bootstrap to AntD. (apache#12738) fix: allow users to reset their passwords (apache#12886) fix(explore): missing select when groupby without metrics (apache#12890) refactor: dbapi exception mapping for dbapi's (apache#12869) feat(style-theme): add support for custom superset themes (apache#12858) chore(lint): fix pre-commit error (apache#12884) refactor(color-schemes): refactor setting of color schemes (apache#12857) feat(native-filters): Add defaultValue for Native filters modal (apache#12199) feat(release): add github token to changelog script (apache#12872) fix(menu): always show settings dropdown (apache#12877) Migrates Label component from Bootstrap to AntD. (apache#12774) [Helm] Automate datasource import (apache#10771) build: Skip loading example data from configs in CI (apache#12610) ...

add required review on master

9701816

superset-github-bot bot added the preset-io label Jan 22, 2021

pull-request-size bot added the size/XS label Jan 22, 2021

nytai approved these changes Jan 22, 2021

View reviewed changes

eschutho changed the title ~~add required review on master~~ chore: add required review on master Jan 22, 2021

eschutho marked this pull request as draft January 22, 2021 20:55

eschutho force-pushed the elizabeth/infra-reviewer-updates branch from 776e227 to 0f06745 Compare January 22, 2021 21:48

eschutho marked this pull request as ready for review January 22, 2021 21:48

update to include all items in required reviews

c057787

eschutho force-pushed the elizabeth/infra-reviewer-updates branch from 0f06745 to c057787 Compare January 22, 2021 21:49

pull-request-size bot added size/S and removed size/XS labels Jan 22, 2021

eschutho commented Jan 22, 2021

View reviewed changes

nytai reviewed Jan 22, 2021

View reviewed changes

ktmud reviewed Jan 22, 2021

View reviewed changes

eschutho mentioned this pull request Jan 22, 2021

chore: elizabeth/revert infra reviewer updates #12698

Closed

6 tasks

eschutho added 2 commits January 22, 2021 15:48

change value on dismiss stale reviews

53aac66

add more action names to checks

9e9bb9a

eschutho commented Jan 26, 2021

View reviewed changes

eschutho commented Jan 27, 2021

View reviewed changes

eschutho force-pushed the elizabeth/infra-reviewer-updates branch from 4712772 to 395fcc4 Compare January 27, 2021 00:29

add just one required action to test

26c6957

eschutho force-pushed the elizabeth/infra-reviewer-updates branch 2 times, most recently from aa60abc to 8cc337c Compare January 27, 2021 20:50

use job name

97b93c2

eschutho force-pushed the elizabeth/infra-reviewer-updates branch from 8cc337c to 97b93c2 Compare January 27, 2021 20:54

eschutho closed this Jan 28, 2021

eschutho reopened this Jan 28, 2021

ktmud approved these changes Jan 28, 2021

View reviewed changes

ktmud merged commit 8553543 into apache:master Feb 3, 2021

villebro deleted the elizabeth/infra-reviewer-updates branch February 3, 2021 07:19

eschutho mentioned this pull request Feb 3, 2021

chore: rename duplicate build jobs #12921

Merged

6 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

chore: add required review on master #12694

chore: add required review on master #12694

Conversation

eschutho commented Jan 22, 2021 • edited by mistercrunch Loading

SUMMARY

ADDITIONAL INFORMATION

eschutho commented Jan 22, 2021

ktmud commented Jan 22, 2021

codecov-io commented Jan 22, 2021 • edited Loading

Codecov Report

eschutho commented Jan 22, 2021

eschutho Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Jan 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nytai Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

ktmud Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

eschutho Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

eschutho commented Jan 27, 2021

eschutho commented Jan 27, 2021

ktmud commented Jan 28, 2021

eschutho commented Jan 28, 2021

nytai commented Feb 3, 2021

ktmud commented Feb 3, 2021

eschutho commented Jan 22, 2021 •

edited by mistercrunch

Loading

codecov-io commented Jan 22, 2021 •

edited

Loading

eschutho Jan 22, 2021 •

edited

Loading

ktmud Jan 23, 2021 •

edited

Loading

nytai Jan 26, 2021 •

edited

Loading

ktmud Jan 26, 2021 •

edited

Loading

eschutho Jan 26, 2021 •

edited

Loading

ktmud Jan 27, 2021 •

edited

Loading