Skip to content
This repository has been archived by the owner on Aug 13, 2019. It is now read-only.

Housekeeping feature branches #347

Open
peterbe opened this issue Feb 15, 2018 · 8 comments
Open

Housekeeping feature branches #347

peterbe opened this issue Feb 15, 2018 · 8 comments
Labels

Comments

@peterbe
Copy link
Contributor

peterbe commented Feb 15, 2018

Unless there's a good reason to make feature branches inside the mozilla-services repo, rather than your personal fork, I think we should discontinue the practice.

  • PRs made on the mozilla-services makes two kinds of Travis builds. One "pr" and one "push". There are ways around that in the .travis.yml by the way.

  • PRs made on mozilla-services triggers a Circle build. (By the way, why do we have Travis and Circle??)

  • There's less sense of "ownership" to topic branches in the main repo. You'd have to open each one and look at the last committer to find out who it belongs to. And it might get messy if every developer uses mozilla-services instead of their own fork.

  • The branches you have in the main repo should be, if there's a need, more "operational" branches like "next" or "dev" or "gh-pages" or "legacy" or "proddeploys".

Opinions @leplatrem @glasserc @Natim @mostlygeek @magopian ?

@Natim
Copy link
Contributor

Natim commented Feb 15, 2018

  • We have circle-ci builds to builds and upload the Docker image for OPS
  • Unlike you I think it is better to have both PR and Push tests in travis because you want to know the status of your branch as well as the status of your branch once merged
  • Same for circle ci I'll better know if it will fail before merging.
  • It is easier to collaborate on branches if there are made in the repository, you don't have to mess with multiple remotes or figure out on which fork they are.

There's less sense of "ownership" to topic branches

I honestly don't think this is an issue, there are no such things as branch ownership, either a branch is part of the operational step or linked to an unmerged pull-request or it can be deleted.

@glasserc
Copy link
Contributor

I think most of these arguments are logistical (why do we have CircleCI? what does this travis build represent?) and the one that remains is about taste: do you prefer to have your own fork, or to share origin. I don't mind too much if other people want to share origin as long as I can work on a fork.

Personally, I prefer the "everything happens on a fork" style, but it's one of many Github workflow disputes that I gave up on prosecuting even two jobs ago. Not everyone likes to have their own fork. I disagree with most of Rémy's points but IMHO "feature branches on origin" is a pretty minor annoyance in a world where some people don't believe in feature branches. Specifically, my feeling is that the main annoyance is that inevitably there are a bunch of branches that are created on origin and you are going to get all of them when you do git pull. I just delete as many as I can on my personal fork when I first create it, and try to ignore as much as possible the branches on origin.

I'd like to highlight Rémy's comment that if a branch is linked to an unmerged PR it can be deleted. I disagree with this statement, both because closed PRs have branches that probably ought to get cleaned up eventually, and because often it isn't obvious whether a branch is "leading towards" a PR, and deleting it prematurely is also problematic. I think there's a larger issue of cleaning up old code, which like most housekeeping tasks tends to be neglected. I think "everything on your fork" style can skirt this issue, but not really solve it (when do you delete a person's fork?).

@peterbe
Copy link
Contributor Author

peterbe commented Feb 15, 2018

We have circle-ci builds to builds and upload the Docker image for OPS

Why not just one? All in on Circle? Then we don't need to mess with .travis.yml quirks and .cricle quirks.

Unlike you I think it is better to have both PR and Push tests in travis because you want to know the status of your branch as well as the status of your branch once merged

Your branch "once merged" is what's being tested in the "pr" build. No? Basically answering, "If you'd merge this topic branch into master and run a build, would it pass?"

Or, does it test the topic branch if it was merged into what master was at the time of making the branch?? If so, I'm stunned. And I'm eager to learn too, but this close to the weekend I'm not going to dig deep into the documentation :)

It is easier to collaborate on branches if there are made in the repository, you don't have to mess with multiple remotes or figure out on which fork they are.

Mind you, if I work with you on a repo, and you make PR that is 99% complete but lacks on final detail. Then you go on vacation, so you can't complete that remaining 1%. Then, because we're both collaborators on the repo, I can push to your fork. It's quite a new feature in github.
Basically, multiple people can push to a forked topic branch. Not just the owner of the fork.

... there are no such things as branch ownership.

But there is. If I do git fetch Natim and git fetch glasserc I can see ALL branches for these people and they're "namespaced" by user. If all branches as in origin, and I do git branch -a I can't decipher which belongs to who.

Remy, I feel like I "pick" on your arguments. Don't worry. I love you and I'll still try to come back to Rennes some day and eat crepes with you.

@leplatrem
Copy link
Collaborator

TravisCI vs. CircleCI : didn't know about CircleCI before Ops asked us to add it in order to build the Docker images. We never took the time to migrate the Travis tests, both stayed. Feel free to show us the way if you're comfortable with this. (same in mozilla/doorman, Kinto/kinto-dist)

About branches... As long as nobody pushes to master, I personally don't care too much. I usually push branches on the main repo because I'm lazy and I don't see any major problem with that. I usually prefix the branch name with the issue number, or in extreme case with my nickname when I'm goofing around (leplatrem-poc-cache-perfs). And I systematically delete branches when PR is merged. As Remy pointed out, I find it convenient when my coworker can push a commit (like this one for example) instead of going through an extra review cycle or oblige him to wait for me to come back from vacation.

Nonetheless, this doesn't prevent us to have reserved branch names like prod, dev or whatever. BTW we never had to have those. Master is always stable and we release everything we deploy. I saw only one exception in 3 years (kinto webextensions postgresql perfs hotfix).

If we care about the wasted resources with circle/travis extra runs, then we can configure them differently.

If you feel motivated to enforce forked repos — and feel ready to tell us off when our habits come back — then please go ahead! I won't fight against it (other hills to die on etc.)

@magopian
Copy link
Contributor

I can more of less mirror @leplatrem's thoughts, so won't add anything more to the argument.

TL:DR; I don't really care one way or the other ;)

@Natim
Copy link
Contributor

Natim commented Feb 16, 2018

Don't worry @peterbe I like it and your arguments makes a lot of sense.

@mostlygeek
Copy link
Collaborator

mostlygeek commented Feb 16, 2018

I am definitely the progenitor of "use circleCI", since I wrote the original Dockerflow specification. CIrcleCI was (is?) a lot better than travis at building and shipping Docker containers. As part of the Dockerflow spec we should:

  1. build our code into a container
  2. test container works correctly (running the tests within the container)
  3. publish the container to Dockerhub

Having two CIs is messy and it's not an argument that's fun to have over and over again. So I'm happy as long as we can automate shipping a container that ops can use in production.

I think having the same habits and conventions as a team is more important that individual workflow habits. It makes it easier to work together. I'm happy to let the team decide but if I was to use my dictator hammer I'd suggest as a minimum:

  • follow our Project Management Standard
  • use feature branches on the origin repo
  • PR work into master, which should always be stable
  • use git tags to tag releases, this triggers CircleCI to build/test/publish a container

This process can be even more strict. Though implementing more processes should be driven by the needs of the project more than how we prefer to work individually.

For example, more processes that sometimes make sense:

  • keep a maintenance branch for every major release (for bug/security fixes, long term support)
  • all commits / tags must be GPG signed
  • must have multiple reviewers for code
  • etc. etc.

@peterbe
Copy link
Contributor Author

peterbe commented Feb 21, 2018

I'm going to do two things:

  1. Move our Project Management Standards document to GitHub and Markdown. Then we can use GitHub pull requests (with its glorious commenting system) to break apart/build up each "opinion".

  2. Propose, that we don't push topic branches to the origin repo but to your personal fork.

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

No branches or pull requests

6 participants