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

[DECIDED] Always run all checks for pack CI #7

Closed
blag opened this issue Apr 17, 2020 · 12 comments
Closed

[DECIDED] Always run all checks for pack CI #7

blag opened this issue Apr 17, 2020 · 12 comments
Assignees
Labels

Comments

@blag
Copy link

blag commented Apr 17, 2020

For pack CI builds, we currently try to only run checks on files that have changed in the PR. This is arguably faster, but it comes at the cost of increased complexity, and a higher incidence of related bugs.

It would be much simpler to just always run all tests on all files for packs, regardless of what or how many have changed. This will be slower, but I don't think it would be particularly slower for almost all packs. The only pack that might have issues is the AWS pack since that's got 3000+ actions that all would get checked on every push, but even then that only takes a few minutes to complete.

For merges into master and the scheduled weekend tests, we use FORCE_CHECK_ALL_FILES to check all files in every pack.

Does anybody know why we originally tried to restrict what we tested in PRs? @LindsayHill @Kami

@blag blag added enhancement New feature or request proposal labels Apr 17, 2020
@blag blag changed the title Always run all checks for pack CI Proposal: Always run all checks for pack CI Apr 17, 2020
@punkrokk
Copy link
Member

Is there any doc that describes the desired outcomes in general for packci?

I have quite a bit of experience running my own exchange in the past year, and one think I always wondered, once I figured everything out, is what was the desired outcome of the ci process. I think it’s to prevent bad code from getting into production, especially in community ( plus listing and stuff) but I think an important part of refactoring it is to understand, let’s say, the goals as of know vs. how they might be changed.

@nmaludy
Copy link
Member

nmaludy commented Apr 18, 2020

I'm in support of this. Seen a lot of PRs around problems with the selective checks. Always checking everything leaves a lot less room for error.

@arm4b
Copy link
Member

arm4b commented Apr 18, 2020

I don't remember the reasons behind why we did selective checks in StackStorm-Exchange CI. Maybe dig into Github history? Was that changed at some point or is it designed that way since the beginning?

If no important reasons, +1 as well to move to "check all" approach to reduce complexity & edge cases.

@punkrokk
Copy link
Member

I’m curious with the newer circle credit based payment if this changes the economics significantly.

@arm4b
Copy link
Member

arm4b commented Apr 20, 2020

BTW we can share the current StackStorm org plan (paid) with the child StackStorm-Exchange org plan (free) in CircleCI so they'll use extra Linux containers.

@punkrokk
Copy link
Member

punkrokk commented Apr 20, 2020 via email

@blag
Copy link
Author

blag commented Apr 20, 2020

If we can get extra (free) Linux containers on CircleCI, that would be great.

@blag
Copy link
Author

blag commented Apr 21, 2020

Okay, tracking this down...

FORCE_CHECK_ALL_FILES came from 49121223daf5ad8edae62f145df5027357031b93, which was part of PR 1 "Add st2contrib Makefile", which also pulled the git utilities from st2contrib.

Digging through st2contrib, the check for FORCE_CHECK_ALL_FILES was added to the git utilities by @Kami in 92438c0c1a9de4a77d67cdb6627d029916f2e5b8 as part of PR 343 "Speed up makefile checks", where it was noted:

If we get too many false negatives in the future (passes in PR, but fails in master), we can always revert those changes.

However, that PR actually pulled in code from the original PR 332, when the packs tests were still all running on Travis. Reading through that PR, it did change the build times a bit:

Travis job Old Time New Time Comparison
TASK=flake8 24 sec 46 sec Slightly slower
TASK=pylint 10 min 40 sec (build failed) 1 min 2 sec (build passed ✅ ✅ Much faster [1]
TASK=configs-check 1 min 11 sec 57 sec Slightly faster
TASK=metadata-check 27 sec 57 sec Slightly faster
TASK=packs-tests 2 min 54 sec 51 sec ✅ Faster

[1] This looks like it's comparing apples and oranges though, since the old build may have failed due to timeout and the new build completed successfully.

I think it did save the original coders an appreciable amount of time when they were on Travis. But I think CircleCI is just slower overall, since it launches each step separately, and that seems to eat up the most time nowadays. The only pack that's build times will be greatly extended by this proposal is the AWS pack. And to be fair, that pack does seem to see more churn than other packs, but it also has more actions by 1-3 orders of magnitude compared to other packs. I think for all non-AWS packs this isn't going to matter much, and I think the reduced complexity is worth the slightly longer build times.

@LindsayHill
Copy link

I think the reduced complexity is worth the slightly longer build times

Agreed. Make the change. Have seen too many oddities in the past

@arm4b arm4b changed the title Proposal: Always run all checks for pack CI [DECIDED] Always run all checks for pack CI Apr 21, 2020
@arm4b
Copy link
Member

arm4b commented Apr 21, 2020

Good stuff here with the Discussion, Proposal, root-cause finding and also new solution like applying for FOSS plan for StackStorm-Exchange github org 👍

@blag I assume you're driving the https://circleci.com/docs/2.0/oss/ for https://github.com/stackstorm-exchange/ ?

@blag
Copy link
Author

blag commented May 12, 2020

Sorry for not replying here. The StackStorm-Exchange organization on CircleCI was already on their OSS plan - yay!

@arm4b
Copy link
Member

arm4b commented May 12, 2020

I guess the current plan for StackStorm-Exchange is just a plan that applies to any org with non-private repositories. No action is needed for that.

Our current plan for StackStorm-Exchange is 25K credits/week and 1 concurrent job according to CircleCI dashboard.

While https://circleci.com/docs/2.0/oss/#overview lists 100K credits/week for OSS plan and 4 concurrent jobs:

To support the open source community, organizations on Github or Bitbucket will be given 100,000 free credits per week that can be spent on on open source projects. These credits can be spent on Linux-medium resources. Each organization can have a maximum of four concurrent jobs running

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants