search

spacepy / dbprocessing

Automated processing controller for heliophysics data
5 stars 4 forks source link

Figure out processing for up-to-date tests, etc. #40

Open jtniehof opened 3 years ago

jtniehof commented 3 years ago

Since we have "Require branches to be up-to-date before merging" set, that gives a button "Update branch." What this appears to do is make a merge commit that merges master into the PR branch for the sake of running the tests. That leaves the PR with non-linear history. You can see this at https://github.com/jtniehof/dbprocessing/commits/unixtime_day_fix , which is the branch for #38. That commit disappears when doing the rebase for the final merge to master, as you can see in the master history at https://github.com/spacepy/dbprocessing/commits/master . I think it probably would disappear even if there were commits after it in the PR history, as long as there weren't conflict merges. It does make it a bit harder to read the pull request (of course, so does a rebase and force-push with all the commits suddenly showing back up.)

I think the options here are:

  1. Continue to use the button and trust that GitHub will keep history from going non-linear.
  2. Continue to require branches to be up-to-date but don't use that button, do a rebase and force-push to the branch.
  3. Remove the GitHub check that requires branches to be up to date, but still as a matter of policy require it (so rebase and force-push.)
  4. Don't require the branch to be up to date. This is what's done in SpacePy. On SpacePy, Travis makes an internal merge commit from master before running the checks: 
    
This is a pull request build.

It is running a build against the merge commit, after merging #x: description. Any changes that have been made to the master branch before the build ran are also included.


But looking at the CircleCI logs, it just pulls from the PR branch and does no further merging.

### Closure condition
This issue should be closed when one of the options above is chosen, written up in the developer doc, and the relevant PR merged.
jtniehof commented 3 years ago

I suppose option 5 would be "option 4 and switch to Travis", but I think there should be a much longer list of things Travis does better than CircleCI before pulling that switch.

EDIT: I had this backwards the first time....

dnadeau-lanl commented 3 years ago

Actually when I merged I had a "beige" button up-to-date. I did not see the Rebase and merge until I press it. I personnally prefer #3. I am not ready to go to Travis just because of a merge. I also looked at CircleCI and might update our postgresql database from Docker after I make the authentication work.

jtniehof commented 3 years ago

Yep, the "Require branches to be up-to-date before merging" setting doesn't allow the rebase-and-merge if the base of the PR isn't current master. It's a bit weird because the "bring it up to date" button makes a merge commit in the PR's branch, at which point the rebase merge is permitted. Removing that check would remove that button, but would make it possible to rebase-merge a PR that isn't up to date, and thus the final state hasn't been explicitly tested.

One of the things we can do that will help keep that from being too bad a thing is setting up regular tests against master, daily or weekly builds. Then if the final product fails tests we'll know sooner. That'll also catch cases where the CircleCI or other environment changes without us having a PR open, which would have helped catch what bit us with #39.

Now that I look at the CircleCI logs, doing option 1 (building against these merges) means it shows up in the CircleCI logs as being done on master, which isn't nice.

Ditching CircleCI isn't really a serious suggestion, but I phrased that poorly.

jtniehof commented 3 years ago

I had a thought: we should be able to update the CircleCI build job to merge master in to the branch just on its checkout. That won't affect the PR branch in the submitter's repository, but would ensure a test against the combined master + PR, as Travis does.

Even better, though, would be to update the build job to rebase against master. That's what happens when we press the "rebase and merge" button on Github anyhow. So if that happens cleanly in the workflow, and the unit tests then pass, there's no need for the PR to be up-to-date against master--we'll have tested the exact end state. If it doesn't happen cleanly, then the build job fails, workflow fails, check fails, and there needs to be a manual rebase-and-force-push. In that situation, Github probably wouldn't allow the rebase and merge anyhow (it won't do a conflict rebase.)

So that's my proposal now: remove the up-to-date check, do not require PRs to be up to date (just to rebase cleanly against master), update CircleCI to rebase the PR against master before install/test. I still want to add the time-based builds against master, too, but that becomes orthogonal to this.

jtniehof commented 3 years ago

Note to self to add the "Allow edits from maintainers" checkbox to the PR checklist as part of this, since it's fairly related.

jtniehof commented 2 years ago

Because of this, #53, and #76, I'm seriously considering switching from CircleCI to GitHub Actions. I think all three of these would be instantly fixed if we switched. The one thing we would give up is access to a shell on the runner (haven't found a way to do that in GHA, although https://github.com/marketplace/actions/debugging-with-ssh is promising).