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

Myoldmopar · 2023-08-28T19:18:48Z

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" ™️

Myoldmopar

Walkthrough complete.

Myoldmopar · 2023-08-28T19:20:15Z

ci.rb

    b.query_pull_requests
+    b.query_branches


Reordered the calls here. Each of these functions just do two independent things, and the order does not appear to matter at all, as they don't really check anything common between them. I moved the PRs check first though because now it collects a list of "internal" PRs with their branch and their PR number. Then when we call for branches next, we have that list ready, and can use that PR number.

Myoldmopar · 2023-08-28T19:20:39Z

lib/build.rb

@@ -32,6 +32,9 @@ def initialize(token, repository, max_age)
    @potential_builds = []
    @max_age = max_age
    github_check_rate_limit(@client.last_response.headers)
+    # Keep a mapping of branch name -> PR number for PRs which are based on non-fork branches
+    # This is only used for providing the GitHub status comment, nothing else
+    @branch_to_pr_number = {}


Just create an empty hash on the build class when it's constructed at the beginning of each CI pass.

Myoldmopar · 2023-08-28T19:22:11Z

lib/build.rb

@@ -66,7 +69,10 @@ def query_branches
            login = branch_details.commit.author.login
          end

-          @potential_builds << PotentialBuild.new(@client, @token, @repository, nil, b.commit.sha, b.name, login, nil, nil, nil, nil, nil)
+          associated_pr_num = @branch_to_pr_number.fetch(b.name, nil)


The PR check should have been called first, which would have populated this hash.

These lines simply:

Get the PR number for this branch, or nil

Create a new PotentialBuild instance as before

Call a method to assign the pr number to the potential build instance

Add the potential build to the list of potential builds

Myoldmopar · 2023-08-28T19:23:09Z

lib/build.rb

@@ -118,6 +124,8 @@ def query_pull_requests

          if p.head.repo.full_name == p.base.repo.full_name
            $logger.info("Skipping pull-request originating from head repo: #{p.number}")
+            @branch_to_pr_number[p.head.ref] = p.number


This PR check should be called before the branch check. This function has always skipped "internal" PRs, where the branch lives inside NREL/EnergyPlus. Now it still does that...but before skipping, it grabs the branch name and PR number to keep them for later.

Myoldmopar · 2023-08-28T19:24:08Z

lib/potentialbuild.rb

@@ -84,6 +84,11 @@ def initialize(client, token, repository, tag_name, commit_sha, branch_name, aut
    @valgrind_counters_results = nil
    @perf_counters_results = nil
    @file_sizes = nil
+    @pr_num_for_comment = nil


Creates a nil member variable to store the PR number that should be used ONLY for posting the GitHub comment.

Myoldmopar · 2023-08-28T19:24:17Z

lib/potentialbuild.rb

+    @pr_num_for_comment = nil
+  end
+
+  def assign_pr_num_for_comment(pr_num)


Small worker function to update the member variable.

Myoldmopar · 2023-08-28T19:24:53Z

lib/potentialbuild.rb

@@ -936,7 +941,9 @@ def post_results(compiler, pending)
      end

      if !pending && @config.post_results_comment
-        if !@commit_sha.nil? && @repository == @config.repository
+        if !@pr_num_for_comment.nil?


OK, so this section has nothing to do with the CI mechanics, just posting the GitHub comment. If the PR number member variable is present, we should go ahead and use that, otherwise fall back to the normal mode.

Myoldmopar · 2023-08-28T19:25:12Z

spec/build_spec.rb

@@ -249,13 +249,16 @@ def configuration
  def pull_request?
    @pr
  end
+  def assign_pr_num_for_comment(pr_num)
+    @pr_num_for_comment = pr_num
+  end


Small worker function for the mock class.

Myoldmopar · 2023-08-28T19:25:20Z

spec/build_spec.rb

 end

 describe 'Build Testing' do
  context 'when calling query_branches' do
    it 'should include branches that are valid and new' do
      allow(Octokit::Client).to receive(:new).and_return(DummyClient2.new)
-      allow(PotentialBuild).to receive(:new).and_return(true)
+      allow(PotentialBuild).to receive(:new).and_return(DummyPotentialBuild.new('dummy'))


Modified mock return value.

jmarrec

This looks decent, considering that you want to build a branch rather than a pull. Building a pull would be more logical to me but whatever.

FWIW, this is going to always skip the first commit it builds on any PR, unless you're fast enough.

Push the branch -> CI starts
Go to github to open the PR -> there is a pull number to post comments on

If CI picks up the branch before the PR is open, then you won't get the comments.

Myoldmopar · 2023-10-17T19:11:29Z

@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 · 2023-10-17T19:24:01Z

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 · 2023-10-18T17:42:03Z

@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 · 2023-10-18T18:10:53Z

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:

Reducing our maintenance burden on Decent CI itself
Eliminating the constant scans of all the branches of E+
Lowering my stress level when I see E+ has like 80-90 branches (because who will care?)
Making it so much easier to comment on the PRs, since everything will be PR-centric

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 · 2023-10-18T19:51:11Z

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)

Minimal fixes to try to comment on the PR itself

fce89e8

Myoldmopar commented Aug 28, 2023

View reviewed changes

Remove unused accessor

b8e1af7

jmarrec reviewed Sep 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Myoldmopar commented Aug 28, 2023

Myoldmopar left a comment

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

Myoldmopar Aug 28, 2023

jmarrec left a comment

Myoldmopar commented Oct 17, 2023

jmarrec commented Oct 17, 2023 •

edited

Loading

Myoldmopar commented Oct 18, 2023

Myoldmopar commented Oct 18, 2023

jmarrec commented Oct 18, 2023 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

Myoldmopar commented Aug 28, 2023

Myoldmopar left a 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

Choose a reason for hiding this comment

jmarrec left a comment

Choose a reason for hiding this comment

Myoldmopar commented Oct 17, 2023

jmarrec commented Oct 17, 2023 • edited Loading

Myoldmopar commented Oct 18, 2023

Myoldmopar commented Oct 18, 2023

jmarrec commented Oct 18, 2023 • edited Loading

jmarrec commented Oct 17, 2023 •

edited

Loading

jmarrec commented Oct 18, 2023 •

edited

Loading