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

Adding Protected Branches and Code Owners to the Manual #51

Merged
merged 15 commits into from
Dec 1, 2017

Conversation

hollenberry
Copy link
Contributor

Hey @githubtraining/trainers + @JasonEtco !

This PR resolves https://github.com/github/services-training/issues/476, which was intended to determine how we might add Code Owners to the manual.

Review

This is not a complete work, but I'm pausing to ask a few questions before diving in further.

  • Does this feel like the right place for Code Owners? I haven't added it to any book path, but the intent would be to place this directly before a workflow discussion.
  • Is a walkthrough the right format, or would a brief description suffice?
  • I know that we cover Protected Branches verbally. Do these two fit together?
  • How should we format Protected Branches?

If we do decide to continue with this format, I'd love for anyone to feel comfortable diving in with commits.

Remaining Steps

  • Add summary and walkthrough or activity for Protected Branches
  • Complete remaining Code Owners activities
    • We probably need to prompt each user to add a friend as collaborator to their repository.
    • I've been using this blog post and this example to populate the content.
  • Reviews of all kinds

@crichID
Copy link
Contributor

crichID commented Aug 31, 2017

Does this feel like the right place for Code Owners? I haven't added it to any book path, but the intent would be to place this directly before a workflow discussion.

@hollenberry given this is a new section, I think that means we can put it anywhere 😉. I wonder if it makes more sense to talk about it with Code review though? Right now we are using protected branches but we aren't really talking about them so I do agree it is also a gap.

Is a walkthrough the right format, or would a brief description suffice?

I think this depends on whether we intend to demonstrate this feature in class. If we intend to demonstrate it then we should do a walkthrough. If we don't then I think using a callout like we do when we discuss switching to a remote branch would be more appropriate.

@hollenberry
Copy link
Contributor Author

@hollenberry given this is a new section, I think that means we can put it anywhere 😉. I wonder if it makes more sense to talk about it with Code review though? Right now we are using protected branches but we aren't really talking about them so I do agree it is also a gap.

Great idea, @crichID . Do you think this should be a separate page immediately following https://githubtraining.github.io/training-manual/GH4D/07_collaborating_on_code.html, or should we incorporate these sections within that page? I think a good point for consideration is that separate pages will allow us to keep a condensed version of the manual in the future as topics like these grow in future 🚢 s and updates.

I think this depends on whether we intend to demonstrate this feature in class. If we intend to demonstrate it then we should do a walkthrough.

👍 👍 👍
@githubtraining/trainers I'd love at least two of your thoughts as well before moving forward on this PR (all 3 of you would be best, since this would be a process change).

@brianamarie
Copy link
Contributor

Right now, we use protected branches in class but don't discuss them at the time in the manual.

I think it makes sense to add a small callout in the manual after they open their first pull request in the class repository but aren't able to merge it. We could just put some brief info like, "You can't merge right now because the master branch is protected! Here's a few sentences about how you can have secure branches with protected branches and code owners, and here's a link to the docs for more detailed information". I don't think an added activity is necessary, but if we ever wanted to add one, I think this activity is ✨ .

@hollenberry @crichID Does that align with how you're thinking about adding code owners and protected branches to the class and manual?

@crichID
Copy link
Contributor

crichID commented Sep 13, 2017

Does that align with how you're thinking about adding code owners and protected branches to the class and manual?

@brianamarie I think this is perfect! It introduces the concept and points them to Help docs for evergreen information.

@beardofedu
Copy link
Member

This PR comes at a great time, both Protected Branches (which is something we have been covering in class without documentation in the manual as stated earlier in this thread) and Code Owners (!!!!!!!!!) were asked about / covered in our class with Discover today.

@beardofedu
Copy link
Member

@hollenberry added protected branch content ... feels a little long in the tooth and I still haven't covered the Include administrators option. I wonder if we should just point them to existing Help documentation on Protected Branches

image

@crichID
Copy link
Contributor

crichID commented Sep 20, 2017

I wonder if we should just point them to existing Help documentation on Protected Branches

@beardofedu I'm a big fan of this idea. In my mind (and I'm willing to be overruled) we should probably only include "steps" in the manual when we will expect the student to follow those steps. In this case, we are explaining actions they can take on their own repositories, but will not take during class. Those types of explanations seem better suited to a callout that discusses "why you should do this thing" or "how you can duplicate this behavior in your project" with a link to Help docs.

@hollenberry
Copy link
Contributor Author

I'm a big fan of this idea.

@crichID @beardofedu works for me! Do we want to keep Code Owners, or set both as pointers to docs?

@beardofedu
Copy link
Member

@hollenberry added a note to the protected branches section about the limit (100) per repo based on this, I think that format will automatically turn it into a "note".

@crichID
Copy link
Contributor

crichID commented Nov 22, 2017

@hollenberry thank you for pointing us to this conversation - I knew we had discussed it but didn't remember where ❤️ I made a few edits as follows:

  1. Minor tweak to make the intro sentence a little more inclusive of the features
  2. Simplified the branch protection activity to only show basic protection and then point the user to the docs for more details on the different settings - I think this will help us keep this section up-to-date more easily than including explanations in the activity.
  3. Added a few examples of how CODEOWNERS might be used.
  4. Simplified the CODEOWNERS activity and changed it to a .js file so we don't get anyone stuck needing a review from githubteacher 😉
  5. Added the step to enable CODEOWNERS in the protected branches settings.

I made these edits thinking it would make the most sense to add this to the github-games activity since there are currently no branch protections set in that repo. Thoughts @githubtraining/trainers?

@brianamarie
Copy link
Contributor

I made a few more commits to:

  • Add this section to the manual
  • Add a title so the style matches other pages

screen shot 2017-11-27 at 10 46 56 am

@githubtraining/trainers What do you think?

Copy link
Contributor

@crichID crichID left a comment

Choose a reason for hiding this comment

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

@brianamarie thank you for getting this so close to 🚢ing. One small comment below:

@@ -30,6 +30,7 @@
* [Viewing Local Changes](17_view_local_changes.md)
* [Changing the `.yml` file](34_change_yml.md)
* [Tags & Releases](17_tags_and_releases.md)
* [Protected Branches & CODEOWNERS](17_protected_branches.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

@brianamarie what would you think of covering this just before we make a change. Perhaps after updating the README and reverting inde.html, but before the atomic commit exercise? That way they could see CODEOWNERS working at least once. Thoughts?

@brianamarie
Copy link
Contributor

what would you think of covering this just before we make a change. Perhaps after updating the README and reverting inde.html, but before the atomic commit exercise? That way they could see CODEOWNERS working at least once. Thoughts?

@crichID I think this is a great idea! Commit made ✔️

@beardofedu
Copy link
Member

I really like the idea of introducing how to create a CODEOWNERS file in the training ✨ @brianamarie

@hectorsector
Copy link
Member

👍

@allthedoll
Copy link
Contributor

🌮 :shipit:

@brianamarie
Copy link
Contributor

@hollenberry Yay! Once it looks good to you, I think you should do the honors and ship 🚢 🎉

@hollenberry
Copy link
Contributor Author

🎉 Thanks @brianamarie ! Shipping.

@hollenberry hollenberry merged commit 6d8841f into master Dec 1, 2017
@hollenberry hollenberry deleted the protected-branches-code-owners branch December 1, 2017 18:30
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.

None yet

6 participants