| === bswinnerton6 is now known as bswinnerton | ||
| blackboxsw | oops. don't know how long that channel was closed. | 15:51 |
|---|---|---|
| blackboxsw | smoser, rharper, Odd_Bloke falcojr I saw a comment on one PR that made me feel like PR merging is unclear now that "committers" (upstream maintainers) are more distributed. My understanding is that any committer should be able to squash merge a branch approved by a cloud-init "committer" per https://cloudinit.readthedocs.io/en/latest/topics/code_review.html . | 15:54 |
| blackboxsw | The only addtional process we'd been trying to shepherd folks into is to start adding more tests/integration_tests coverage for new features to make SRU easier. | 15:55 |
| blackboxsw | in otubo's case, https://github.com/canonical/cloud-init/pull/721 I wanted to avoid putting that burden on him because I held up this review so long, so I'm taking an action item to write up an integration test for this to help with whenever the next SRU is. | 15:56 |
| blackboxsw | smoser, rharper, OddBloke, falcojr: But, I had one general question, are we ready to start documenting, and supporting PR authors in writing integration tests as a prerequisite for each PR? https://cloudinit.readthedocs.io/en/latest/topics/code_review.html#prerequisites-for-landing-pull-requests | 15:57 |
| blackboxsw | right now that doc only mentions unit-tests as prereq. | 16:00 |
| blackboxsw | https://github.com/canonical/cloud-init/pull/721 merged. Thanks again otubo/smoser for all the back and forth and detailed review there. | 16:08 |
| powersj | blackboxsw, saw smoser ask this, but may have missed the answer, when merging, what are we suppose to do with the proposed commit message? | 16:15 |
| blackboxsw | powersj: ahh, right thanks. For new PRs we generally expect to copy and paste the "Proposed Commit Message" section from the PR.description at the top. Maybe that's worth making it clearer in https://cloudinit.readthedocs.io/en/latest/topics/code_review.html too. | 16:18 |
| powersj | agreed, and to echo smoser's request it would be nice if that were automated or at least linted in some manner | 16:19 |
| blackboxsw | +1 on that powersj. we could instrument a simple linter status_check on the PR.description Proposed Commit Message section to early warn if linting wasn't honored | 16:19 |
| blackboxsw | so we don't have to manually sort that at merge time.\ | 16:20 |
| blackboxsw | ....or if we just had a merge bot/action that'd automate landing and commit msg setting on approval | 16:20 |
| blackboxsw | get the humans out of the equation during merge | 16:20 |
| falcojr | blackboxsw: to your question about integration tests, yes, I have been asking for them from community PRs | 17:08 |
| hamalq_ | hi can i got +1 https://github.com/canonical/cloud-init/pull/860 | 17:10 |
| hamalq_ | another i did this change https://github.com/canonical/cloud-init/pull/859/files but i dont see the test for rhel in there is there another place i should modify for test | 17:37 |
| blackboxsw | hamalq_: approved https://github.com/canonical/cloud-init/pull/860#pullrequestreview-624544953 | 17:49 |
| blackboxsw | awaiting CI then will merge | 17:49 |
| hamalq_ | blackboxsw: thanks | 17:50 |
| === paride3 is now known as paride | ||
| === jmcgnh_ is now known as jmcgnh | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!