Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

RogerHowellDfE
Copy link
Member

@RogerHowellDfE RogerHowellDfE commented Jan 11, 2023

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!

@RogerHowellDfE RogerHowellDfE requested a review from a team as a code owner January 11, 2023 11:55
@RogerHowellDfE
Copy link
Member Author

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.

@RogerHowellDfE
Copy link
Member Author

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?)

Comment on lines 122 to 127
### 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/)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Comment on lines 122 to 127
### 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/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### 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/)

Copy link
Member

@peteryates peteryates left a 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.

@pritchyspritch
Copy link
Contributor

@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.

@RogerHowellDfE RogerHowellDfE force-pushed the feature/review_and_update_storing_source_code_page branch from b32d180 to 6577913 Compare August 5, 2024 09:52
@saliceti
Copy link
Member

saliceti commented Aug 5, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants