| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-09-21 | |||
| 15:23:03 | sean-k-mooney | not on commit locally | |
| 15:23:15 | clarkb | it would be better to run it after the commit like the gerrit chaneg id hook | |
| 15:23:30 | clarkb | then people won't ask me why push is so slow to gerrit | |
| 15:23:53 | sean-k-mooney | yes so if you configure it to run pre-commit on push instead of commit then you get that effect | |
| 15:23:59 | sean-k-mooney | but its not the default | |
| 15:24:07 | sean-k-mooney | and i dont think they have a post commit option | |
| 15:24:43 | sean-k-mooney | clarkb: being able to run it on push instead of commit i think is relitively new to the tool | |
| 15:26:21 | sean-k-mooney | https://pre-commit.com/#top_level-default_stages | |
| 15:27:31 | sean-k-mooney | actully you might be able to add addtion stages https://pre-commit.com/#config-stages | |
| 15:27:45 | sean-k-mooney | this is beyond my basic use of the tool to date | |
| 15:28:07 | bauzas | sean-k-mooney: I'm not super happy either with pre-commit hooks | |
| 15:28:29 | bauzas | sometimes I just wanna push | |
| 15:28:38 | sean-k-mooney | bauzas: it removes a lot of the teedium with doing dev | |
| 15:28:41 | bauzas | and I don't want it to be hold per the sake of something I don't care | |
| 15:28:42 | sean-k-mooney | bauzas: you can bypass it | |
| 15:28:54 | sean-k-mooney | you just set an env var on the push line | |
| 15:29:02 | sean-k-mooney | or commit line | |
| 15:29:06 | bauzas | sean-k-mooney: I told about *pre-commit* | |
| 15:29:15 | bauzas | not pre-gerrit ;) | |
| 15:29:22 | bauzas | and sorry the push was meaningless | |
| 15:29:28 | bauzas | I meant commit | |
| 15:29:45 | sean-k-mooney | yes you can overrid it if you just want to skip the checks | |
| 15:30:03 | clarkb | what is the issue with running `tox` ? | |
| 15:31:24 | sean-k-mooney | its slower in some cases and pre-commit is not just about running tests | |
| 15:31:37 | sean-k-mooney | it can fix things like mixed tabs/spaces ectra | |
| 15:32:06 | sean-k-mooney | ye are both going to "love" https://review.opendev.org/c/openstack/nova/+/806182 by the way | |
| 15:32:17 | sean-k-mooney | based on your reaction to date :) | |
| 15:32:18 | stephenfin | bauzas: git commit -n | |
| 15:32:26 | bauzas | reminder : nova meeting in 28 mins-ish | |
| 15:33:08 | bauzas | clarkb: yeah that's one of my concerns | |
| 15:33:19 | bauzas | we push more and more checks into commit stage | |
| 15:33:28 | bauzas | and we add more linters | |
| 15:33:56 | sean-k-mooney | and we improve the quaity of the code and our developemt workflow as a result | |
| 15:34:27 | stephenfin | don't make humans do things that machines are good at | |
| 15:34:39 | clarkb | if tox and pre commit run the same checks they should run in about the same amount of time | |
| 15:35:15 | stephenfin | yeah, pre-commit and 'tox -e fast8' will run in ~ the same time, but you need to remember to do the latter | |
| 15:35:15 | sean-k-mooney | clarkb: in some cases yes | |
| 15:35:16 | bauzas | stephenfin: I don't disagree, I'm just on clarkb's side on the fact that could be left to tox | |
| 15:35:20 | clarkb | stephenfin: the problem is discoverablity. Humans don't know what all the checks are and some of them don't run until you get to zuul (commit message linting). Also some of it is severe nitpicking like having a '.' in the subject line or not | |
| 15:35:36 | bauzas | and linter jobs could be non-voting | |
| 15:35:53 | stephenfin | we have commit message linting? | |
| 15:35:57 | bauzas | we do | |
| 15:36:00 | clarkb | stephenfin: yes that is what started this entire conversation | |
| 15:36:02 | bauzas | for backport checks | |
| 15:36:02 | sean-k-mooney | bauzas: they could be but unless we are going to do that with pep8 im really not ok with that | |
| 15:36:04 | stephenfin | outside of the cherry-picked from line? | |
| 15:36:20 | clarkb | stephenfin: the bot that fixes your .gitreview file when you make a new branch got -1'd because its commit message wasn't good neough | |
| 15:36:31 | sean-k-mooney | clarkb: yep | |
| 15:36:45 | sean-k-mooney | the both should have had a stable only statement in that commit | |
| 15:36:54 | clarkb | the bot predates the check :P | |
| 15:37:12 | sean-k-mooney | i know but its not the first time we have modfied the bots patch | |
| 15:37:13 | stephenfin | clarkb: which change is this, specifically? | |
| 15:37:35 | sean-k-mooney | stephenfin: the one for the .gittreview file for the stable branch | |
| 15:37:39 | stephenfin | for nova? | |
| 15:37:42 | sean-k-mooney | yes | |
| 15:37:48 | clarkb | https://review.opendev.org/c/openstack/nova/+/809759 | |
| 15:38:46 | stephenfin | okay, that's what I was expecting to see. Where's the check for the '.' in the subject line? | |
| 15:38:52 | stephenfin | Or is that another project? | |
| 15:39:24 | clarkb | stephenfin: that was an old checker for hacking. I bring it up because after much fighting we finally conceded that any checks on commit messages are a bad idea because nothing checks them until it is too late | |
| 15:39:40 | stephenfin | Ah, okay, thanks. I'm caught up now :) | |
| 15:39:41 | clarkb | basically any linter checks for commit messages are doomed to fail | |
| 15:39:48 | clarkb | because nothing runs the linters post commit | |
| 15:39:53 | clarkb | (except for zuul) | |
| 15:40:20 | sean-k-mooney | clarkb: by the way if i am runging tox its often after i commit | |
| 15:40:45 | clarkb | ok I don't know anyone else that does that unless they are bisecting to find issues in already merged code | |
| 15:40:55 | stephenfin | I agree that we should carve out an exception for these bot generated changes to stable | |
| 15:41:06 | sean-k-mooney | or just fix them | |
| 15:41:12 | clarkb | there is a semi related issue here with PBRs support for encoding semver requirements in commit messages | |
| 15:41:41 | clarkb | that causes a ton of problems too because commit messages will be merged and pbr will disagree with the version someone tagged by hand and then you end up i na really weird spot | |
| 15:42:02 | clarkb | (basically you need to be flexible with commit messages because you creat headaches when you try to enforce too many rules around them) | |
| 15:42:12 | stephenfin | sean-k-mooney: they've always been that way though so it seems weird to let our custom script dictate what the commits for every other project looks like | |
| 15:43:05 | sean-k-mooney | i semi agree but on the ohter hand hacking allows things i hate and blocks things i like yet we allow it to keep the porject semi consitent | |
| 15:43:14 | lyarwood | I've likely missed something here but why does this matter if a core still needs to ACK the bot proposed patches? | |
| 15:43:33 | lyarwood | and in ACK'ing these bot changes can add the [stable-only] tag? | |
| 15:43:36 | sean-k-mooney | stephenfin: when we added the script we were hoping ti would gain adoption in other porjects too | |
| 15:43:51 | lyarwood | it's not like these changes are merge automagically right? | |
| 15:43:55 | lyarwood | merged* | |
| 15:44:04 | sean-k-mooney | correct | |
| 15:44:18 | sean-k-mooney | i dont see why the stable team cant fix the bot patches when they approve | |
| 15:44:31 | clarkb | the whole point of having the bot is to reduce human overhead | |
| 15:44:34 | stephenfin | clarkb: Agreed in general. This feels slightly different though. It was put in because dansmith was bothered by me proposing backports to multiple branches at once, in the fear that they'd merge in the wrong order or a patch higher up would be modified and the changes wouldn't be captured in later patches. Those still seems like sensible concerns that'd be easily missed by reviewers | |
| 15:44:44 | clarkb | essentially we've got two bots that are in theory supposed to reduce overhead but in reality dobule it | |
| 15:45:03 | lyarwood | well that's on the stable team who want the second bot surely? | |
| 15:45:11 | lyarwood | I don't get the argument here tbh | |
| 15:45:31 | lyarwood | we want the cherry-pick job and are happy with the overhead | |
| 15:45:37 | lyarwood | surely that's enough? | |
| 15:45:47 | clarkb | I've got two main concerns. The first is that after many yaers we still act like proposing a non backport change to stable branches is immediately wrong. This is frustraing for people like me who basically only do that when touching stable branches | |
| 15:46:09 | clarkb | the second issue is that any linter applied to commit messages is problematic because you don't typically run linting post commit | |
| 15:46:10 | sean-k-mooney | im more or less of the opipion that we keep the cherry pick lines and enforce them in a job or we done enforce them in a job and dont require them any more | |
| 15:46:55 | sean-k-mooney | clarkb: well actully having the bot tell you its wrong to propose directly to stable was one of the goals of the script | |
| 15:47:12 | sean-k-mooney | clarkb: its not that uncommon to have one off patches propsoed directly to stable | |
| 15:48:07 | sean-k-mooney | i have -1'd patches a cople of time for that and explained that the issue need to be resolved in master first | |
| 15:48:39 | elodilles | clarkb: just for the record, this cherry-pick-check is only in nova repository (well, and afaik cyborg adopted it as well), all the other repositories are not having it | |
| 15:53:36 | lyarwood | clarkb: sorry back, I appreciate that the initial -1 is slightly off putting but again if the stable team looking after the repo are happy to do the work to fix this while they approve then I can't see an issue here. | |
| 15:53:47 | opendevreview | Stephen Finucane proposed openstack/nova master: tools: Ignore bot-generated branch creation patches https://review.opendev.org/c/openstack/nova/+/810285 | |
| 15:54:04 | stephenfin | lyarwood, sean-k-mooney, elodilles: eh? ^ | |
| 15:54:25 | gibi | stephenfin: thanks! | |
| 15:54:28 | stephenfin | no need to backport that, obv. It'll just avoid this discussion come stable/yoga creation | |
| 15:54:28 | dansmith | stephenfin: can we not just agree on some flag for bot-generated things? | |
| 15:54:34 | lyarwood | ha hackaroundFinucane has entered the chat | |