| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-09-21 | |||
| 14:58:26 | gibi | which is the rule, with exceptions | |
| 14:58:50 | bauzas | but I think I already said that a lot (context: mypy) | |
| 14:59:05 | elodilles | well, the point with that extra check is to ensure we have the *correct* patch backported | |
| 14:59:15 | bauzas | if linters block us instead of helping us, then it's a problem | |
| 15:00:27 | elodilles | as it sometimes happened that a patch was backported to several branch at the same time and later on modified (a middle patch) and those changes were not backported to older branches | |
| 15:00:28 | gibi | bauzas: what if linters both help us but sometimes blocking us | |
| 15:01:21 | elodilles | bauzas: don't look at that linter as a blocker but as a helper to catch human failure :) | |
| 15:04:11 | elodilles | about the .gitreview patch needed manual change: IIRC we had a prompt discussion in the past and decided that it's not a big deal to update those 2 patches every six month | |
| 15:07:22 | clarkb | elodilles: right I get that, the issue is occasionally (and usually when I'm touching stable) we have to modify the mechanics of the branch directly without a backport | |
| 15:07:49 | clarkb | this is an example of that case. I think reviewers can see that an update to .gitreview to set the correct branch or update a git review option is fine without a backport | |
| 15:10:49 | elodilles | clarkb: actually i also do stable fixes that are stable only and for those cases we (nova (and some other teams)) use 'stable-only' tags in commit message | |
| 15:11:27 | clarkb | elodilles: yes but that isn't consistent and you never updated the bot to do that (which is really my complaint) | |
| 15:12:10 | clarkb | you should disable the bot on your repos or update the bot | |
| 15:12:23 | elodilles | clarkb: ok, i see your point | |
| 15:12:29 | clarkb | personally I'd remove the lint rule entirely because the only way to discover the rule is to push code and have it fail first | |
| 15:12:30 | elodilles | clarkb: I'll update the bot | |
| 15:14:23 | elodilles | clarkb: well, in our team stable cores decided that this linter is useful for us (especially for reviewers). I understand your point, but i tend to look it from another perspective :) | |
| 15:17:17 | clarkb | ok. In general I think commit message linters are a bad idea. Users don't know they will fail until after they have pushed (because typical workflow is make changes, run tox, commit, push) | |
| 15:19:05 | sean-k-mooney | clarkb: we tried to strick a blance by having it non votign in check to give early feedback and block it only in gate | |
| 15:19:06 | elodilles | clarkb: without the linter we did the very same: if a stable reviewer saw that something is not complete in the commit message (or a wrong patch was backported) then we asked the author to change the patch | |
| 15:19:44 | sean-k-mooney | they can also run the lint check locally | |
| 15:19:45 | clarkb | elodilles: right but in the case of fixing the mechanics of a stable branch you wouldn't do that because it is fine as is | |
| 15:19:45 | gibi | clarkb: I think it is not impossible to add the current check as a tox target too (or part of pep8) so the extra push can be avioded | |
| 15:19:50 | elodilles | clarkb: and that linter (script) can be also run locally | |
| 15:19:51 | sean-k-mooney | most user do not backport | |
| 15:19:56 | clarkb | sean-k-mooney: they can but literally no one expects they have to run the linter after committing | |
| 15:20:07 | clarkb | this is why we removed the '.' at end of subject lines rule from hacking | |
| 15:20:28 | sean-k-mooney | clarkb: well we have been trying to encurage people to use precommit more by the way | |
| 15:20:37 | clarkb | oh wow. I would -2 that too :P | |
| 15:20:39 | elodilles | gibi: we have it as an extra tox target now as stephenfin proposed it a couple of months ago | |
| 15:20:49 | clarkb | (it doesn't handle dependencies properly and you'll completely bypass mirrors etc) | |
| 15:21:09 | sean-k-mooney | clarkb: pre comiit is not a repleace fdor any of our ci | |
| 15:21:24 | sean-k-mooney | it just elimnates the excuse fo forgetting to run expected checks | |
| 15:21:24 | clarkb | sean-k-mooney: right but people run it locally and may have proxies or mirrors etc | |
| 15:21:51 | clarkb | sean-k-mooney: the pre commit hook won't catch the commit issue errors though because it happens too soon right? | |
| 15:22:13 | sean-k-mooney | in this case yes | |
| 15:22:24 | sean-k-mooney | but it will run flake8 exctra | |
| 15:22:42 | sean-k-mooney | it is too early for this backport check | |
| 15:22:53 | sean-k-mooney | althogh technially | |
| 15:22:59 | sean-k-mooney | you can configure it to run on push | |
| 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 | sean-k-mooney | clarkb: in some cases yes | |
| 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: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 | 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:02 | bauzas | for backport checks | |
| 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 :) | |