search

nhsx / open-source-policy

Open Source Policy development for the NHS
Other
47 stars 11 forks source link

Coding in the open versus requirement for code review - paradox? #19

Closed wbryant closed 2 years ago

wbryant commented 2 years ago

The policy states that by default "All new code should be: Coded In The Open", but also that "an internal code review should be conducted for all open source projects before publication".

Is there any guidance on how to navigate this?

connor1q commented 2 years ago

Hi William, We have this guidance: https://github.com/NHSDigital/rap-community-of-practice/blob/main/development-approach/09_code-review.md

wbryant commented 2 years ago

Thanks - for clarity if I make a feature on a branch when coding in the open, it would be visible as soon as I push it? So even if there is a code review prior to merging, it will be visible prior to that? Or is there a step I am missing?

connor1q commented 2 years ago

Different teams have different approaches. For us, we would push the branch to remote - so others in the team can see it, pull it to their local machine, and run the code. At this phase, we try to encourage the bulk of code review - treating it as a collaborative design process where everyone is still happy to make substantial changes to the code.

Then when we are happy the code is ready to be merged into the main branch we do a merge request. This might prompt automatic running of tests, linters, etc that will reject the request if they fail. The request to merge to master also prompts someone senior to review before approving.

We try to keep short lived feature branches which makes this whole process quite lightweight. There is never that much change in each merge so we keep the cadence high and everyone on the same page. Also fewer merge conflicts

connor1q commented 2 years ago

To more directly answer your question - visibility will depend on how you have set up your repo. I understand that github steers you towards being fully open or fully closed. So any work-in-progress in a branch would be visible. I know some teams who get around this by doing development in a private repo. Then whenever they merge to main they transfer that to the open repo

wbryant commented 2 years ago

That makes good sense - I may not have appreciated it while I was reading through but is this spelled out in the policy - or is it more appropriate to reference the NHSD RAP community where more practical guidance on how you organise this sort of thing could be held?

otlah commented 2 years ago

Hi both, I'll double check there are no other conflicts (there shouldn't be) and then I'm very happy to link out to the guidance above!

otlah commented 2 years ago

Checked and linked!

wbryant commented 1 year ago

So hypothetically if I push up some data to remote (and it is not caught automatically for whatever reason) it would be public prior to review? Or are the branches held on private repos, and merged to the main branch on the public repo only after review?

From: connor1q @.> Sent: 11 May 2022 09:30 To: nhsx/open-source-policy @.> Cc: William Bryant @.>; Author @.> Subject: [External Email] Re: [nhsx/open-source-policy] Coding in the open versus requirement for code review - paradox? (Issue #19)

Different teams have different approaches. For us, we would push the branch to remote - so others in the team can see it, pull it to their local machine, and run the code. At this phase, we try to encourage the bulk of code review - treating it as a collaborative design process where everyone is still happy to make substantial changes to the code.

Then when we are happy the code is ready to be merged into the main branch we do a merge request. This might prompt automatic running of tests, linters, etc that will reject the request if they fail. The request to merge to master also prompts someone senior to review before approving.

We try to keep short lived feature branches which makes this whole process quite lightweight. There is never that much change in each merge so we keep the cadence high and everyone on the same page. Also fewer merge conflicts

— Reply to this email directly, view it on GitHubhttps://github.com/nhsx/open-source-policy/issues/19#issuecomment-1123346956, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AB7LEJNIBIUOQ37MCV55533VJNVZZANCNFSM5VS7F6YQ. You are receiving this because you authored the thread.Message ID: @.***>

Disclaimer The information contained in this communication from the sender is confidential. It is intended solely for use by the recipient and others authorised to receive it. If you are not the recipient, you are hereby notified that any disclosure, copying, distribution or taking action in relation of the contents of this information is strictly prohibited and may be unlawful.