Jenkins pipelines and their governance

Developers
1

Hey all (especially @syclik),

I’m much of the way through translating our Jenkins job configurations into declarative pipelines stored in Jenkinsfiles, similar in idea to Travis’s .travis.yml though with a very Groovy syntax. Among other seemingly considerable benefits, this means our testing configuration will now be stored in the relevant repo (and thus outsiders can easily see how we test our code, and we can easily see how a certain version of our code was tested).

But our governance for the repo typically requires a full code review process, and our governance for the Jenkins config previously did not require this for equivalent changes. I would like to appeal to the devs here to ask permission to change these files without code review (we can still use a pull request so we can test that the changes have the desired effect, but once the tests pass with the new config we could merge). Does anyone mind this? Governance characteristics will be the same as the old system where Jenkins config could be changed by any admin at will, but a little safer because now the tests will run on the new config.

I think this will be especially helpful as I’m porting over our old configs and learning about pipeline syntax, since there have been many iterations and the back-and-forth of code review, while normally a nice quality control procedure on our actual code, I think could be safely skipped in CI land.

I’m hoping mostly for @syclik’s approval/engagement but wanted this thread to be public in case anyone had any major considerations.

Thanks,
Sean

2

@seantalts, mind moving this to the public Developer’s category? I didn’t see anything here that should be marked private. And this sort of governance / process discussion should be in the open.

3

It’s public now. The Jenkins machines are a Columbia resource and I’m not sure others care about their maintenance but here we are.

4

I think there are at least two different things to discuss here. I don’t think it quite falls under governance. Here are the two things I see:

  1. What should we code review?
  2. What does GitHub allow us to do?

What should we code review?

The point of our process it to make sure our source code is in a good state on the develop branch. We should be free to change our process if it’s getting in the way or isn’t enforceable.

I think we should:
A. Continuous integration changes: test that the CI works, but no review required. (Of course if it’s requested, we should get help!)
B. GitHub template updates: review, no testing.
C. Doc changes: code review and build doc.
D. Source code: code review and test. How much we test can be dependent on what the change is, what it might impact, and what we think we need to test to keep develop in a good state. (In a distributed development team, it’s very, very disruptive when we can’t trust that the source code is in a good state.)

I do think we can move to different levels of testing depending on the change.

For continuous integration changes, we should be confident that the changes work before changing implementations.

What does GitHub allow us to do?

GitHub only gives us repo-level permissions on code review. GitHub has it so either all pull requests on the repo have to be approved or none. They also allow admins to merge without review, but by pressing a big red button.

In theory, I’m ok with no review here. But in practice, the difficulty with allowing admins to merge without review is that mistakes can happen. I’ve made them, I’ve seen almost everyone that’s contributed to the Stan and Math repo make them. It’s really easy to accidentally commit a change that’s unrelated and have things slip by the review process, which isn’t what is intended. (This is really annoying because this also applies to the GitHub templates for pull requests and issues too.)

There are at least a couple ways to handle this. Perhaps we could have the stan-buildbot account automatically approve pull requests when it doesn’t touch source code. Or we can store the configuration in another repo outside of the Stan repo (that’s essentially what’s done now; it’s all visible through Jenkins directly, but harder to get to than the Jenkinsfiles).

I, personally, wouldn’t like some devs to have the authority to merge without review. I think from the outside, there will be some confusion as to who is allowed to do this and under what circumstances. I’d rather just avoid that confusion in the hopes that it prevents accidental breaking of Stan.

5

Thanks!

Re: others not caring. That shouldn’t be the criteria for making things public. All dev-related discussion should be public unless there’s a good reason for it to be closed. Trust me, I’ve had tons of conversations with myself on the dev lists. And sometimes, people chime in with thoughtful discussion when I least expected it!

1 Like
6

That all sounds good. I’d like to keep the Jenkinsfiles in with the code so we’re able to easily match up what tests ran against what commits. I think that anyone with write access (i.e. can approve pull requests) to the repo should also know when it is appropriate to merge without code review? I don’t see this as a public relations issue. If we did set up a bot (as another Jenkins job?) what would the logic be? auto-approve makefile make/* Jenkinsfile and .travis.yml changes?

7

That makes sense. Now we just need to fix the practical problem of the resolution of GitHub’s permissioning.

It’s a lot easier when the rule is simple, like “never” or “always.” In this case, I’d prefer never.

I wasn’t trying to make it a public relations issue. It’s a process issue and one where the process is meant to save us from ourselves. Trust me, we’ll all forget what the rule is if it’s not enforced by some sort of bot and we’ll mess up.

Close. We can auto-approve pull requests if they only contain changes to:

  • Jenkins related config: Jenkinsfile
  • Travis related config: .travis.yml
  • GitHub related things: README.md, .github/
  • Release related? RELEASE-NOTES.txt

We should not auto-approve our make process. That’s a bit too important. I think someone else should put their eyes on it. The impact of having that break is pretty rough.

We can also be smarter about when to run tests. I’m assuming if .travis.yml changes, we don’t need to run Jenkins. If Jenkinsfile changes, we should run everything. If README.md or .github/ changes, we don’t need to test anything. If the only change is the manual, we only need to test the manual. If the language is the only thing that changes, then we only need to test the language.

1 Like
8

I’ll take a shot at setting up Jenkins to do both of these things, I think it might not be too terrible. Do you want to approve the Stan pipelines PR in the meantime and I can get us switched over to pipelines today (at least for Stan, need to run final Math pipeline tests once these go in, which will take ~10 hours).

9

@syclik do you want me to wait for approval for the Stan pipeline PR or should I just merge it? I’m still working on auto approve but want to stop maintaining two pipelines and switch over ASAP.

10

I’ll review it carefully before we switch over. Maybe a couple days?

I want to switch over, but just want to make sure there’s at least a second person on the team that can understand it. And make sure we have the right tests in place.

(This is going to be so much better. I can see it already. Thanks!)

1 Like