Bitbucket staat eigen pull request reviews toe?

[MrScratch]

Wij werken sinds kort met Bitbucket om aan version control te doen. Nu hebben we een repository opgezet met 4 developers. Ik heb ingesteld dat een pull request minimaal 1 review moet hebben. Hierbij ging ik er vanuit dat je niet je eigen pull request zou kunnen approven.

Maar tot mijn verbazing lukt dit gewoon zonder problemen. Iemand enig idee hoe dit kan?

Ik heb zitten zoeken op internet, maar de enige hits die ik krijg gaan juist over mensen die deze feature graag zouden willen, maar waarbij het niet werkt.

[Snake]

Wat zijn de opties hier?

Als een user ingesteld is als 'default reviewer', dan kan hij/zij zijn/haar eigen PR approven.

[Hydra]

MrScratch schreef op vrijdag 19 juni 2020 @ 16:27:
Wij werken sinds kort met Bitbucket om aan version control te doen. Nu hebben we een repository opgezet met 4 developers. Ik heb ingesteld dat een pull request minimaal 1 review moet hebben. Hierbij ging ik er vanuit dat je niet je eigen pull request zou kunnen approven.

Daar maak je dan toch gewoon afspraken over?

[RobIII]

Hydra schreef op vrijdag 19 juni 2020 @ 17:29:
[...]


Daar maak je dan toch gewoon afspraken over?

What he said. Dit wil je niet in techniek maar in procedure en afspraken afvangen. Je kunt wel zeggen dat iemand anders 't moet reviewen en vervolgens blijkt die ander alles blind te accepteren en je bent wéér bij af. Dat je wat regeltjes kunt instellen om 'oopsie, foutje' te voorkomen is gewoon een extra'tje, maar je eigenlijke probleem ligt bij je mensen, hun werkethiek en de afspraken die je hebt en niet bij BitBucket.

[Davidshadow13]

Hydra schreef op vrijdag 19 juni 2020 @ 17:29:
[...]


Daar maak je dan toch gewoon afspraken over?

Absoluut maar ik merk in grotere bedrijven met meerdere teams in dezelfde repository dat je dit wel gewoon technisch moet afdwingen omdat er dan altijd mensen tussen zitten die de kantjes er vanaf lopen of gewoon denken ach joh dit kan er wel even bij.

Afspraken maken sowieso maar zodra je met > 10 devs in een repo zit, of met devs op meerdere locaties dan wil je dat niet alleen procesmatig vastleggen maar ook gewoon technisch beperken zodat deze afspraken afdwingbaar zijn.

[Hahn]

Het grote voordeel van zelf kunnen reviewen/mergen is dat als er écht iets grondig misgaat, en niemand kan helpen, dat je het in je eentje kan oplossen. Dat kan een boel centen schelen en problemen voorkomen bij nood.

[Hydra]

Davidshadow13 schreef op vrijdag 19 juni 2020 @ 17:45:
[...]


Absoluut maar ik merk in grotere bedrijven met meerdere teams in dezelfde repository dat je dit wel gewoon technisch moet afdwingen omdat er dan altijd mensen tussen zitten die de kantjes er vanaf lopen of gewoon denken ach joh dit kan er wel even bij.

Tja. Dan spreek je ze er op aan. Als ik een keer om 6 uur gebeld wordt omdat er echt iets stuk is, wil ik in ieder geval geen technische blokkades om dingen te fixen.

[Hydra]

Hahn schreef op vrijdag 19 juni 2020 @ 17:45:
Het grote voordeel van zelf kunnen reviewen/mergen is dat als er écht iets grondig misgaat, en niemand kan helpen, dat je het in je eentje kan oplossen. Dat kan een boel centen schelen en problemen voorkomen bij nood.

Precies. Dat is m.i. een stuk belangrijkers dan een paar klojo's die de regels blijven negeren. Die geef je uiteindelijk een vriendelijke tik op de vingers.

[Voutloos]

✅✅✅✅✅
Zo, hier heb je een paar reservevinkjes om in te zetten, want ik kan het alleen maar eens zijn met Hahn en Hydra.

Doe mij maar meer dan 90% vd tijd een normaal verlopen PR normaal en een significant vinkje, ipv altijd vinkjes maar dan met een onbekend aandeel omdat-het-moetjes.

Discipline is geen technisch probleem. Dus ook geen technische oplossing.

[Giesber]

Hoewel ik het volledig eens ben dat dat in de ideale situatie niet afgedwongen moet worden ben ik ook al bedrijven tegen gekomen waar dat wel handig was. Daar werden zelfs de tikken op de vinger consequent genegeerd. En als er te veel tikken kwamen gingen ze naar hun baas om te vertellen dat de regels te veel hinderen, en volgde er 3 maanden discussies of reviews wel zinvol waren. Want ja, die baas wist niet genoeg van moderne software ontwikkeling, en hij hoorde enkel het woord hinderen.

Dus ja, afdwingen heeft in sommige situaties wel nut, en is soms bijna noodzakelijk. Of je dan tussen de juiste mensen zit is een andere vraag.

[Gomez12]

Davidshadow13 schreef op vrijdag 19 juni 2020 @ 17:45:
[...]


Absoluut maar ik merk in grotere bedrijven met meerdere teams in dezelfde repository dat je dit wel gewoon technisch moet afdwingen omdat er dan altijd mensen tussen zitten die de kantjes er vanaf lopen of gewoon denken ach joh dit kan er wel even bij.

Afspraken maken sowieso maar zodra je met > 10 devs in een repo zit, of met devs op meerdere locaties dan wil je dat niet alleen procesmatig vastleggen maar ook gewoon technisch beperken zodat deze afspraken afdwingbaar zijn.

Bij grote teams etc kan ik me er iets bij voorstellen, aangezien je dan gewoon meer mensen hebt.

Alleen als je 4 devvers hebt dan voorspel ik dat er minimaal 1 dag per jaar is dat er maar 1 aan het werk is / 3 afwezig zijn en dan mag je dus aan een klant gaan uitleggen dat je zijn hotfix die hij nodig heeft om 100 man aan het werk te houden niet kan doorvoeren, want je zit te wachten op een vinkje...

[Hydra]

Giesber schreef op vrijdag 19 juni 2020 @ 19:33:
Hoewel ik het volledig eens ben dat dat in de ideale situatie niet afgedwongen moet worden ben ik ook al bedrijven tegen gekomen waar dat wel handig was. Daar werden zelfs de tikken op de vinger consequent genegeerd. En als er te veel tikken kwamen gingen ze naar hun baas om te vertellen dat de regels te veel hinderen, en volgde er 3 maanden discussies of reviews wel zinvol waren. Want ja, die baas wist niet genoeg van moderne software ontwikkeling, en hij hoorde enkel het woord hinderen.

Dus ja, afdwingen heeft in sommige situaties wel nut, en is soms bijna noodzakelijk. Of je dan tussen de juiste mensen zit is een andere vraag.

Je bedrijft ook hier weer allemaal issues waar mensen het probleem zijn en er dus geen technische oplossingen zijn.

Dan maar minimaal 1 review afdwingen leidt er alleen maar toe dat je in de problemen komt als er wel een keer snel wat gedaan moet worden.

Daarbij; een organisatie waar management niet de ballen heeft developers die vertikken zich aan de regels te houden daarop aan te spreken, hoef ik niet te werken.

[Kalentum]

Verplichte PR approvals kunnen ook een security maatregel zijn. Ik vind het zelf vergezocht maar het kan voorkomen als er automatisch gedeployed wordt na een merge naar master. Als dan iemand anders toegang heeft tot de laptop van de developer en die kan mergen naar master kan er misbruik van gemaakt worden.

[Hydra]

Kalentum schreef op zaterdag 20 juni 2020 @ 13:29:
Verplichte PR approvals kunnen ook een security maatregel zijn. Ik vind het zelf vergezocht maar het kan voorkomen als er automatisch gedeployed wordt na een merge naar master. Als dan iemand anders toegang heeft tot de laptop van de developer en die kan mergen naar master kan er misbruik van gemaakt worden.

Als iemand 'anders' toegang heeft tot die laptop heb je wel wat grotere problemen dan dat.

[Morax]

MrScratch schreef op vrijdag 19 juni 2020 @ 16:27:
Wij werken sinds kort met Bitbucket om aan version control te doen. Nu hebben we een repository opgezet met 4 developers. Ik heb ingesteld dat een pull request minimaal 1 review moet hebben. Hierbij ging ik er vanuit dat je niet je eigen pull request zou kunnen approven.

Maar tot mijn verbazing lukt dit gewoon zonder problemen. Iemand enig idee hoe dit kan?

Ik heb zitten zoeken op internet, maar de enige hits die ik krijg gaan juist over mensen die deze feature graag zouden willen, maar waarbij het niet werkt.

Hoewel dit kan wordt een approval op je eigen pull request niet meegerekend in de voorwaarde "moet minimaal 1 approval hebben"

[Douweegbertje]

RobIII schreef op vrijdag 19 juni 2020 @ 17:41:
[...]

What he said. Dit wil je niet in techniek maar in procedure en afspraken afvangen. Je kunt wel zeggen dat iemand anders 't moet reviewen en vervolgens blijkt die ander alles blind te accepteren en je bent wéér bij af. Dat je wat regeltjes kunt instellen om 'oopsie, foutje' te voorkomen is gewoon een extra'tje, maar je eigenlijke probleem ligt bij je mensen, hun werkethiek en de afspraken die je hebt en niet bij BitBucket.

Dat kan prima als jouw bedrijf zo erin staat. Er zijn ook bedrijven waar RFC's en nog veel andere processen toonbaar aanwezig moeten zijn. Dus forceren van die processen is dan wel degelijk een ding. Er zit nogal een verschil in wat je doet als bedrijf. Het zijn niet altijd interne processen en/of audits maar net zo goed extern.

Ik snap 100% wat je wilt zeggen hoor, en net zo goed dat ik snap dat papierwerk soms een wassenneus is. Echter zijn er wel degelijk heel wat use-cases waarin dit gewoon logisch is om in plaats te hebben.

[MrScratch]

Snake schreef op vrijdag 19 juni 2020 @ 16:59:
[Afbeelding]
Wat zijn de opties hier?

Als een user ingesteld is als 'default reviewer', dan kan hij/zij zijn/haar eigen PR approven.

Ik had de 1e checkbox aan staan. Maar ik denk dat ik het probleem al zie. We hadden ook write access aanstaan voor iedereen. Dan kan iedereen gewoon alles doen. Dat is natuurlijk niet handig.

[MrScratch]

RobIII schreef op vrijdag 19 juni 2020 @ 17:41:
[...]

What he said. Dit wil je niet in techniek maar in procedure en afspraken afvangen. Je kunt wel zeggen dat iemand anders 't moet reviewen en vervolgens blijkt die ander alles blind te accepteren en je bent wéér bij af. Dat je wat regeltjes kunt instellen om 'oopsie, foutje' te voorkomen is gewoon een extra'tje, maar je eigenlijke probleem ligt bij je mensen, hun werkethiek en de afspraken die je hebt en niet bij BitBucket.

Tuurlijk zijn afspraken het beste, maar dit zijn 4 mensen die in verschillende landen zitten en dat maakt het niet makkelijker We lopen niet ff bij elkaar langs. 1tje is een loose-cannon die nog wel eens een foute actie wil doen en dan zou ik het fijn vinden als hij niet ongewild de master repository kan slopen ofzo. We zijn net met Git begonnen en we snappen eigenlijk nog te weinig van hoe het precies werkt. Dan is het fijn als er geen mogelijkheid is tot onherstelbare schade te doen.

Juist dit lijkt nu gebeurd te zijn. Hij heeft een merge/commit gedaan en ik ben nog aan het uitzoeken wat hij precies uitgespookt heeft.

[RobIII]

Dan is het fijn als er geen mogelijkheid is tot onherstelbare schade te doen.

Dat is nou 't mooie aan een VCS - dat is (haast) onmogelijk. Je kunt altijd terug naar een vorige commit. Tenzij die gebruiker veel te veel rechten heeft (om die repo te droppen bijv.) moet 'ie 't gek maken wil 'ie 't stuk krijgen - en dan nog: als het goed is heb je backups van je projecten / code.

Ik snap overigens dat 't bij sommige bedrijven allemaal wat strakker moet en misschien wel volgens allerlei vastgestelde procedures en whatnot. Mijn punt was: het begint natuurlijk gewoon met een stukje discipline en afspraken. Techniek kan zeker iets toevoegen maar je moet er niet blind op varen.

[Hydra]

MrScratch schreef op maandag 22 juni 2020 @ 10:08:
Tuurlijk zijn afspraken het beste, maar dit zijn 4 mensen die in verschillende landen zitten en dat maakt het niet makkelijker We lopen niet ff bij elkaar langs. 1tje is een loose-cannon die nog wel eens een foute actie wil doen en dan zou ik het fijn vinden als hij niet ongewild de master repository kan slopen ofzo. We zijn net met Git begonnen en we snappen eigenlijk nog te weinig van hoe het precies werkt. Dan is het fijn als er geen mogelijkheid is tot onherstelbare schade te doen.

Juist in dit soort situaties moet je gewoon afspraken kunnen maken. Serieus; als die knakker dit soort dingen doet en zich niet aan de afspraken kan houden; flikker hem eruit voordat hij van de codebase echt een terignzooi maakt.

Dit soort developers die zich niet aan afspraken kunnen houden maken altijd een enorme rotzooi van hun werk en zijn volledig ongeschikt om in een team te werken. Ik zou hierover echt een gesprek gaan houden met je manager. Het is dat; of je bent over een jaar echt compleet klaar met het project en bezig op zoek te gaan naar een andere plek.

[dev10]

MrScratch schreef op maandag 22 juni 2020 @ 10:08:
[...]
Juist dit lijkt nu gebeurd te zijn. Hij heeft een merge/commit gedaan en ik ben nog aan het uitzoeken wat hij precies uitgespookt heeft.

Dat klinkt alsof hij een git push --force gedaan heeft. Succes met uitvogelen wat precies. Dit kun je in BitBucket voorkomen door bij branch permissions 'Allow rewriting branch history' uit te zetten.

Verder wat @Hydra zegt. Als iemand op deze manier met de codebase om gaat: keiharde afspraken maken en anders afscheid van nemen.

Edit: nog een kleine toevoeging: als de persoon in kwestie inderdaad de master branch gesloopt heeft kun je ook altijd nog je lokale kopie terugzetten met git push --force. Als je dat doet en je zet de branch permissies daarna goed, kan persoon in kwestie nooit meer dit soort geintjes uithalen

[ Voor 24% gewijzigd door dev10 op 22-06-2020 13:32 . Reden: Typo... ]

[MrScratch]

Het is meer onwetendheid en onbekendheid met GIT en hoe het werkt. Geen opzet. We zijn allemaal nog aan het uitzoeken hoe we het nu het beste kunnen gebruiken. Het is niet een standaard development project, maar meer een solution configuratie met artifacts die we willen version controllen.

Ik heb nu de write access op de branches uitgezet en alles loopt nu via de pull requests, dus ik denk dat het vanaf nu wel goed gaat.

Overigens stond 'Allow rewriting branch history' al uit, dus dat is er niet gebeurd. Het lijkt alsof hij een feature-branch heeft gemaakt en daar de develop branch in heeft gemerged ipv anderom. Ik begrijp het nog niet helemaal.

[Hydra]

MrScratch schreef op maandag 22 juni 2020 @ 13:36:
Overigens stond 'Allow rewriting branch history' al uit, dus dat is er niet gebeurd. Het lijkt alsof hij een feature-branch heeft gemaakt en daar de develop branch in heeft gemerged ipv anderom. Ik begrijp het nog niet helemaal.

Dat is heel normaal. Af en toe wil je de branch waar je tegenaan merged in je feature branch mergen om op tijd merge conflicten op te lossen. Merge conflicten los je dus op je eigen branch op, voordat je terugmerged.

Alleen moet je niet de dev branch dan deleten. Je kunt normaliter deze langlopende branches protected maken zodat dat niet kan.

[GrooV]

Hier ook altijd minimaal 1 reviewer, en ja dan kan je zelf ook je PR goedkeuren. Echter staat dit wel in de history dus je komt er niet zo maar mee weg. Echter is het soms wel nodig om zelf een PR goed te keuren

PR's worden altijd aangekondigd in een Teams kanaal en wanneer ik hem zelf goedkeur is hier meestal een tijds reden voor. In een team met 3 dev's werk je soms nu eenmaal alleen, of dit nu savonds of op vrijdagmiddag of in de vakantie is.

Je PR's zo klein mogelijk houden zorgt ook dat de boel overzichtelijk blijft, grote PR's zijn eigenlijk not done want je hebt gewoon geen overzicht. Verder goede afspraken maken en iedereen is zelf verantwoordelijk voor zijn code, de reviewer kijkt alleen of er geen gekke dingen in zitten maar die test niet.

Develop in je feature branch mergen is trouwens vaak nodig als je merge conflicten wilt oplossen

[ShitHappens]

Mocht je dit trouwens heel belangrijk vinden, zou je nog kunnen overwegen te upgraden naar Premium. Daarin komt de optie "Prevent a merge with unresolved merge checks" aanzetten, waarmee je die minimum X approvals kunt forceren. In X telt een approval van de PR aanvrager zelf niet mee.
Zonder premium komt er enkel een waarschuwing dat het nog niet de bedoeling is een merge uit te voeren - hij verhindert dus de merge niet.

Wij gebruiken de 'feature' dat je zelf je eigen PR kunt approven om aan te geven dat we zelf de changeset nog eens goed bekeken hebben, of er per ongeluk toch iets in de commits zat dat er niet thuis hoort.

[q-enf0rcer.1]

Gewoon werken met minimaal 2 reviews, en de reviewers ook verantwoordelijk stellen voor fouten in de code die in master gemerged worden. Dan is het snel afgelopen met de mensen die de kantjes er graag vanaf lopen door code direct master in the mergen of reviewers die de code niet eens bekijken en snel op accept klikken :-)