-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Review and update page: "Storing Source Code" #165
base: master
Are you sure you want to change the base?
Review and update page: "Storing Source Code" #165
Conversation
Checks (deployment to temporary instance) are failing due to this PR being sourced from a fork, therefore relevant secrets are (rightly) being withheld from the GItHub Action run. I don't yet have permissions to create / push to a branch on the repo directly. |
Should/could also include a mention of maintaining the integrity of the repo also: e.g., branch protection rules Potential opportunity to also include build/test checks on pull requests and mention the option to specify "required statuses" -- this helps to protect and maintain the integrity of the source code we're storing ... as do other automated scanning tools and update management -- but I fear this is where we definitely creep across the line from being able to include it on a page titled "storing source code" and into "should be on a separate page 😀" (perhaps a page on CI and/or CD?) |
### Work vs Personal GitHub accounts | ||
|
||
You may use your personal GitHub account, but you should: | ||
|
||
- [GitHub: Add your DfE email address to your account](https://help.github.com/articles/adding-an-email-address-to-your-github-account/) | ||
- [GitHub: Use your secondary (DfE) email address for notifications](https://help.github.com/articles/managing-notification-emails-for-organizations/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using personal accounts is pretty much the default rather than something that's allowed. Also not sure we need to be telling people which email addresses to add to their account or how they should receive notifications. Feels a bit prescriptive and I can't really see any benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we have variation across DfE :)
In my local bubble when this got discussed during some folks onboarding, I think the feeling was that work-related notifications wouldn't appear on work devices unless you add your work email address so this became the unofficial suggestion at the time.
Reflecting on it more now, it seems stranger that this would mean that non-work notifications would appear in work inboxes and my personal previously-on-the-fence position has now shifted against doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes, I agree that this should likely be removed or restructured/rephrased.
If we do have any semblance of an official position (e.g., if we encourage use of personal GitHub accounts) then that would be good to include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't really thought of it from a notifications point of view, I disable email notifications and rely on GitHub's blue bell.
If you take a browse through the organisation's people list there are a few accounts containing 'DfE' here and there but the vast majority look like personal ones.
Based on my initial flick through there's a split between digital/non-digital folks, so maybe the guidance should be given at that level.
### Work vs Personal GitHub accounts | ||
|
||
You may use your personal GitHub account, but you should: | ||
|
||
- [GitHub: Add your DfE email address to your account](https://help.github.com/articles/adding-an-email-address-to-your-github-account/) | ||
- [GitHub: Use your secondary (DfE) email address for notifications](https://help.github.com/articles/managing-notification-emails-for-organizations/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Work vs Personal GitHub accounts | |
You may use your personal GitHub account, but you should: | |
- [GitHub: Add your DfE email address to your account](https://help.github.com/articles/adding-an-email-address-to-your-github-account/) | |
- [GitHub: Use your secondary (DfE) email address for notifications](https://help.github.com/articles/managing-notification-emails-for-organizations/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @RogerHowellDfE, happy for this to be merged. I recommend dropping the guidance on work vs personal accounts as either approach is fine.
@RogerHowellDfE @peteryates Any particular reason this never got merged? I was planning on producing some good github standards and guidance, this seems like a good start. |
b32d180
to
6577913
Compare
@RogerHowellDfE the test pipeline doesn't work as the PR was opened outside of this repo. You have write access, do you mind reopening it from this repo? |
I keep flip-flopping on whether this goes beyond the scope of a "review".
Much of it is needed additional information/context/cross-linking, I'm just not sure if it belongs in this specific PR on this specific file vs being done in a separate follow up PR...
Thoughts / comments / feedback (positive or otherwise) strongly encouraged and welcomed!