search

Myoldmopar / decent_ci

0 stars 0 forks source link

Minimal fixes to try to comment on the PR itself #58

Open Myoldmopar opened 1 year ago

Myoldmopar commented 1 year ago

OK, I made a much smaller set of changes this time. I restrained myself from refactoring out release artifact handling and aging PR handling stuff. I kept the changes to as minimal as possible, and it's hard to imagine how this wouldn't "Just Work" :tm:

Myoldmopar commented 1 year ago

@jmarrec are you indicating we should only build pulls? Just don't run CI on branches at all? If so, that's a big change, but I'm open to discussion.

jmarrec commented 1 year ago

My understand of a typical setup in CI systems is to build PRs: the commit you build being the resultant of the merge of the PR branch into the target branch, which is typically what you really care about to know if a branch should be merged. Otherwise you could be building a branch that's based on outdated target branch (develop), which no merge conflicts, that tests just fine, but as soon as your drop the PR the checks fail.

And additionally a couple of "main" branches (main /master + develop), probably running some more checks, and tag event for deploying and so.

Github actions CI this means:

on:
  pull_request:
    branches: [ main, develop ]
  push:
    branches: [ main, develop ]
    # Sequence of patterns matched against refs/tags
    tags:
      - 'v*' # Push events to matching v*, i.e. v1.0, v20.15.10

This is also what OS SDK does on Jenkins. And what I've typically seen done with travis CI in the past as well. And what I typically do on the rare cases I do bitbucket pipelines.

My understanding is that E+ decent_ci builds all branches being pushed.

Myoldmopar commented 1 year ago

@jmarrec thank you. I do understand the real intent of CI is to build PRs that you are wanting reviewed as a step toward merge. Often times with EnergyPlus, devs are working on branches well before opening a PR. Perhaps this just needs to change completely. Maybe we need to stop building all branches, and only build PRs. We do make good use of the Draft PR status as a clue for which PRs are actually ready for review. This step would greatly simplify our CI code. The only change is that devs would need to know they have to open a PR to get Decent's checks. I wonder how everyone would feel about that? I'll ping the team.

Myoldmopar commented 1 year ago

So far 3 positive looking votes on Slack about doing this change, where Decent CI will only respond to PRs. I feel optimistic about this making big positive change:

I'll hold from doing much in here briefly, but when I feel like an hour or two of productive distraction, I'm coming back here.

jmarrec commented 1 year ago

That way you can also run reg tests on each push to develop, in a dedicated folder, and all PRs you build will compare to that one. (My memory is a bit fuzzy on how decent CI does reg tests currently, but I think I recall some inefficiencies there)