| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-25 | |||
| 15:19:39 | efried | Agree with that; though I have to say I'm not a fan of commit titles like "Follow on for <change-id>" with nothing in the body of the message. | |
| 15:20:10 | efried | When I do these, I tend to do "FUP for <brief description>" and then enumerate links to specific comments I'm responding to in the body. | |
| 15:20:16 | efried | and of course, everybody should do it my way. | |
| 15:20:57 | sean-k-mooney | haha well that at least makes sense | |
| 15:21:23 | efried | example: https://review.openstack.org/#/c/600474/ | |
| 15:22:44 | sean-k-mooney | i think this is fine too https://review.openstack.org/#/c/639099/ but a midpoint betwen the too is praobly good too. | |
| 15:23:14 | kashyap | efried: Yeah, I fully agree on "bad commit messages". Reminds me of this excellent guide written by DanPB many moons ago: https://wiki.openstack.org/wiki/GitCommitMessages | |
| 15:23:23 | kashyap | People should take that guide to the heart. | |
| 15:23:24 | sean-k-mooney | the disadvantage of both are you have to click the link/change id to see the context | |
| 15:24:02 | sean-k-mooney | kashyap: there are some thing in the bad examples that i dont fully agree with in that guide but i do reference it frequently | |
| 15:24:03 | kashyap | (And of course this excellent post from a certain Chris Beans: https://chris.beams.io/posts/git-commit/) | |
| 15:24:41 | kashyap | sean-k-mooney: The fundamental rule I follow in commit messages is: provide all the context right there in the commit message. | |
| 15:24:45 | efried | kashyap: I frequently reference the GitCommitMessages wiki page. (That link showed up purple for me :) | |
| 15:24:47 | kashyap | If you have to link to something, summarize it | |
| 15:25:20 | kashyap | (E.g. faciliate the poor suckers working without internet connection and are doing `git log` sleuthing) | |
| 15:25:34 | kashyap | efried: Cool :-) | |
| 15:25:53 | kashyap | sean-k-mooney: That point is actually point 2 in that Wiki page: "Do not assume the reviewer has access to external web services/site." | |
| 15:25:56 | sean-k-mooney | kashyap: one thing that i always tought that wiki discuoraged was bullet point list in the commit which i think are actully good style | |
| 15:26:14 | sean-k-mooney | yep | |
| 15:26:37 | kashyap | sean-k-mooney: Bullet points are fine -- as long as they are complete, and _coherent_ sentences | |
| 15:26:44 | kashyap | And not no-assed "thoughts" | |
| 15:26:48 | sean-k-mooney | so i think https://review.openstack.org/#/c/638058/2//COMMIT_MSG is a good commit message but that general styple would be discuraged | |
| 15:27:29 | kashyap | sean-k-mooney: Was actually reading that change | |
| 15:28:14 | efried | My understanding is that what's discouraged is changing more than one thing in a commit. A bullet list sometimes (but definitely not always) indicates that that's happening. But a bullet list isn't inherently bad. | |
| 15:28:41 | efried | be like saying, "never use the word 'also' in a commit message" | |
| 15:29:00 | sean-k-mooney | efried: ya that is true the distinciton is not made clear in the example in the wiki | |
| 15:29:31 | kashyap | sean-k-mooney: I have a small 'issue' with that vscode commit message | |
| 15:30:08 | kashyap | It doesn't follow the general "scheme" most of the human brains are used to: describe the problem, then tell the solution. | |
| 15:30:10 | sean-k-mooney | kashyap: there are 2 thing i need to fix | |
| 15:30:26 | sean-k-mooney | kashyap: so leave a comment and ill adress them shortly | |
| 15:30:26 | kashyap | Lastly, this is my Git commit message 'scheme': | |
| 15:30:26 | kashyap | [One line summary -- in imperative mood] | |
| 15:30:26 | kashyap | Describe the problem. | |
| 15:30:28 | kashyap | Describe your solution. And more importantly, tell *why*. | |
| 15:31:18 | kashyap | (You do that, though. But all of them in bullets :-)) Anyway, don't want to belabor on this. | |
| 15:32:08 | stephenfin | bauzas: Fancy doing me the honour? https://review.openstack.org/#/c/445436/ | |
| 15:32:59 | bauzas | stephenfin: with pleasure | |
| 15:33:43 | bauzas | stephenfin: oh wait, sec. the option is *already* deprecated ? | |
| 15:33:52 | bauzas | stephenfin: if so, it should have a reno note | |
| 15:33:56 | bauzas | if not, we need it | |
| 15:34:03 | stephenfin | that's a good point | |
| 15:34:09 | stephenfin | lemme fix that | |
| 15:35:36 | bauzas | stephenfin: will update the commit msg to stop the zuul check then | |
| 15:35:52 | stephenfin | If you remove the -W, it'll take it out of the gate | |
| 15:35:56 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: conf: Improve documentation for defer_iptables_apply https://review.openstack.org/445436 | |
| 15:37:35 | bauzas | stephenfin: nope, unless zuul changed | |
| 15:37:58 | bauzas | for pushing the change out of the gate pipeline, we need to have a new revision | |
| 15:38:05 | bauzas | anyway, it's done | |
| 15:46:03 | openstackgerrit | Lajos Katona proposed openstack/python-novaclient master: Add support for microversion v2.70 https://review.openstack.org/637234 | |
| 15:54:43 | openstackgerrit | Stephen Finucane proposed openstack/nova master: conf: Deprecated 'defer_iptables_apply' https://review.openstack.org/445436 | |
| 15:54:46 | stephenfin | bauzas, jaypipes: That should be better ^ | |
| 15:55:27 | stephenfin | sean-k-mooney tells me what I'm doing is likely correct and I'm going to b̶l̶a̶m̶e̶ ̶h̶i̶m̶ ̶i̶f̶ ̶I̶'̶m̶ ̶w̶r̶o̶n̶g̶ trust him :) | |
| 15:56:18 | jaypipes | heh | |
| 15:59:23 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add unit tests for missing VirtualInterface in 2.70 os-interface https://review.openstack.org/639141 | |
| 16:00:02 | mriedem | can i get another core on the 'expose virtual device tags' api change? https://review.openstack.org/#/c/631948/ alex +2ed it and it's pretty straight-forward. last day in the runway queue for this one. | |
| 16:00:14 | stephenfin | sean-k-mooney: My docs are "nice to have". I feel cheated, Good Sir ;) | |
| 16:00:20 | stephenfin | mriedem: I can take a gawk | |
| 16:00:25 | mriedem | thanks | |
| 16:01:37 | yonglihe | this one in same situation: https://review.openstack.org/#/c/621474/22 | |
| 16:01:38 | sean-k-mooney | stephenfin: docs chagnes are non fuctional and can can be merged after the client lib freeze. since we generally just use the latest version of the docs people should still see them but they also can be backported to stable branches more easily so nice to have | |
| 16:02:08 | sean-k-mooney | stephenfin: hehe but i also think they will be straigt forward to merge so im going to go review them shortly anyway | |
| 16:02:16 | mriedem | yonglihe: i'll try to get to that one | |
| 16:03:23 | yonglihe | thanks. | |
| 16:04:26 | yonglihe | i suppose all stuff piled up at end of dev cycle, sorry for that. | |
| 16:12:37 | mriedem | np, it always happens | |
| 16:20:50 | bauzas | stephenfin: +Waboom | |
| 16:25:33 | openstackgerrit | Merged openstack/nova master: Refactor "networks" processing in ServersController.create https://review.openstack.org/633594 | |
| 16:27:31 | stephenfin | bauzas: Tank u | |
| 16:28:23 | stephenfin | .--._____, | |
| 16:28:23 | stephenfin | .-='=='==-, " | |
| 16:28:23 | stephenfin | (O_o_o_o_o_O) | |
| 16:33:29 | yonglihe | -:) | |
| 16:40:09 | openstackgerrit | Stephen Finucane proposed openstack/nova-specs master: Add 'flavor-extra-spec-image-property-validation' spec https://review.openstack.org/638734 | |
| 16:42:36 | openstackgerrit | Matt Riedemann proposed openstack/nova master: [Doc] Best practices for effectively tolerating down cells https://review.openstack.org/638173 | |
| 16:43:11 | tssurya | thanks mriedem ^ | |
| 16:43:15 | mriedem | np | |
| 16:57:49 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Fup for the bandwidth series https://review.openstack.org/639159 | |
| 16:58:10 | gibi | mriedem: your comments for the bandwidth series are fixed in ^^ | |
| 16:58:54 | gibi | jaypipes, efried: thanks for the comments in the bandwidth series, I will update https://review.openstack.org/639159 with your comments as well, possibly tomorrow | |
| 17:00:44 | artom | Object equality in tests in a PITA | |
| 17:00:48 | artom | *is | |
| 17:01:11 | artom | Expect call: Flavor(<some stuff>), Actual call: Flavor(<exact same stuff>) | |
| 17:03:49 | artom | *facepalm* | |
| 17:03:54 | artom | No, that's not i :( | |
| 17:03:58 | artom | *it | |
| 17:04:46 | sean-k-mooney | stephenfin: left some comments on you os-vif docs changes most are minor | |
| 17:07:24 | sean-k-mooney | artom: object equality check pending change so you need to reset changes on both object before comparing them | |
| 17:08:06 | sean-k-mooney | otherwise you get X != X issues | |
| 17:10:23 | jaypipes | artom: what sean-k-mooney said is almost always the problem with that. | |
| 17:10:54 | sean-k-mooney | jaypipes: artom we proably should just create a function in the base tescae for comparing objects | |
| 17:11:13 | jaypipes | sean-k-mooney: there was one somewhere I think... maybe dansmith can remember :) | |
| 17:11:22 | sean-k-mooney | e.g. self.assertObjEquals | |
| 17:12:30 | sean-k-mooney | jaypipes: would it break the world if we changed __eq__ in base ovo to ignore the changed state of fields? | |
| 17:12:46 | sean-k-mooney | i assume yes since we have not done so before | |
| 17:12:50 | artom | sean-k-mooney, jaypipes, yeah, so it this case it was dumber than that - my main problem was I had an - and an _ in my fake values | |
| 17:13:02 | sean-k-mooney | oh :) | |
| 17:13:11 | artom | But once that hurdle was over, there was still an object I had to replace with a fake string. | |
| 17:13:45 | artom | But yeah, a more intelligent way of comparing objects would be super | |
| 17:13:46 | jaypipes | hah :) | |
| 17:14:18 | artom | It wouldn't even be that hard - implement __eq__ in the base fields, and then recursively compare fields | |