Skip to content

Conversation

@jakebailey
Copy link
Member

@jakebailey jakebailey commented Sep 1, 2020

These jobs run on any push to master/main, including on forks, leading extra runs and notifications for simple things like pulling upstreams. The workflows also only run on master/main, which means they don't provide any early feedback for people submitting PRs (unless you're submitting your PR from the default branch, which is a no-no anyway).

There's no way to mark an entire workflow to only run on a specific repo (boo), so add the if statement to each of the jobs that don't already have it.

If someone does actually want these checks temporarily, they can modify their config and remove it before PRing, but I personally don't expect anyone to do that.

@jakebailey jakebailey added the no-changelog No news entry required label Sep 1, 2020
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@kimadeline
Copy link

@brettcannon I've seen that topic before 😂

#13547 (comment)

image

So in the end we don't want the workflow to run on every fork?

@brettcannon
Copy link
Member

brettcannon commented Sep 1, 2020

@kimadeline Jake doesn't want to run in every fork's master/main, so I think saying "we" is a bit strong 😉 . I'm fine with starting like this for Jake's benefit and then we can consider rolling it back if it turns out only Jake doesn't want it this way (I know he says it's a "no-no" to use one's main branch for PRs, but it's surprisingly common in my experience).

@codecov-commenter
Copy link

Codecov Report

Merging #13733 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13733      +/-   ##
==========================================
- Coverage   59.82%   59.80%   -0.03%     
==========================================
  Files         675      675              
  Lines       37796    37799       +3     
  Branches     5445     5446       +1     
==========================================
- Hits        22612    22605       -7     
+ Misses      14026    14018       -8     
- Partials     1158     1176      +18     
Impacted Files Coverage Δ
src/client/datascience/crossProcessLock.ts 79.41% <0.00%> (-11.77%) ⬇️
src/client/common/utils/platform.ts 56.00% <0.00%> (-4.00%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0.00%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0.00%> (-1.57%) ⬇️
src/client/common/process/proc.ts 14.49% <0.00%> (-0.73%) ⬇️
src/client/common/configSettings.ts 69.28% <0.00%> (-0.38%) ⬇️
src/client/logging/levels.ts 61.11% <0.00%> (ø)
src/client/logging/logger.ts 62.74% <0.00%> (ø)
src/client/logging/formatters.ts 53.57% <0.00%> (ø)
src/client/logging/transports.ts 65.11% <0.00%> (ø)

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 59b8bb2...35a55b4. Read the comment docs.

@jakebailey
Copy link
Member Author

jakebailey commented Sep 1, 2020

IMO the vast majority of PRs are submitted by the team, who aren't submitting their PRs from master/main... And running it on every fork just seems like a ton of wasted builder time.

@jakebailey
Copy link
Member Author

I'm honestly a bit surprised that only us three have an opinion... :)

@brettcannon brettcannon merged commit 8194efb into microsoft:master Sep 1, 2020
@brettcannon
Copy link
Member

@jakebailey it might be because I don't think most people bother syncing up their main branch in their fork (I personally never bother and just pull straight from upstream since my fork's copy will never be the canonical version of that branch).

@kimadeline
Copy link

(I do sync origin/main and upstream/main lol)

@jakebailey jakebailey deleted the disable-workflows-in-forks branch September 3, 2020 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants