search

pixelated / pixelated-user-agent

User facing components of Pixelated: a JavaScript single page app and a RESTful service.
GNU Affero General Public License v3.0
159 stars 72 forks source link

How to deal with LEAP forks? #687

Closed deniscostadsc closed 8 years ago

deniscostadsc commented 8 years ago

We started a discussion about how we manage our LEAP forks[1].

The main point is that we use to create a PR(Pull Request) from our main branch Doing this we do not can work on that branch until the PR is merged. If we still work on this branch all changes are also going to the PR.

One way to solve this problem is work on code for PRs on a separated branch and create the PR from that branch. After PR is created who is developing can merge the branch in the main branch, so the team can still work with that changes and dont mess with the already submited PR.

But this approach has a problem, if I want to work on the same library to submit another PR before the previous one been accpeted, in order to dont have not-accepted-code-yet on my work I have to checkout to the parent commit of that PR.

One approach to deal with this is to have two "main branches". One for been exactly equal to LEAP develop branch, and other one to have our code that still waiting for acceptance.

        not
        accepted
        PR
        yet
        branch        develop

         O             O
         |             |
         |             |
         |             |
         O             O
        /|
       / |
      /  |
     O   |
    /    |
  PR1*   |
    \    |
     \   |
      \  |
       \ |
        \|
         O

* PR1 is a branch just for that PR.

This approach needs some careful steps. We cant rebase to the 'not-accepted-PR-yet' branch. Otherwise we are going to mess with the history and we wont be able to check if we already have develop on our other "main branch".

But I'd like to ask you about this. Is this really necessary? Dont you think that overwork to manage something very simple?

We do that just because it takes time to have PR accepted on LEAP and because their too long release cycle. Dont you think that the PR reviewing time is OK now? The recent PR the took a long time to be merge are complex PRs.

About the argument about Leap taking too long to release ther libs, we talked with Kali and Meskio, and they told us they can release more often, if this is going to help us.

Do we still need have forks fo that libs?

I'like to discuss a new method to manage PR on our forks, but I'd appreciate if you take a time to think on this.

[1] - https://github.com/pixelated?utf8=%E2%9C%93&query=+only%3Aforks

:heart:

tuliocasagrande commented 8 years ago

I think we should first ask ourselves if we really need our forks - or at least if we are using our forks in the right way. We have dependencies in our projects pointing to pixelated forks, which are very inconsistent with leap repos. Our leap_platform, for instance, is 14 commits ahead, 54 commits behind leapcode.

deniscostadsc commented 8 years ago

Our leap_platform, for instance, is 14 commits ahead, 54 commits behind leapcode.

I think this is the main problem. This shouldn't happen, ever.

varac commented 8 years ago

Our leap_platform, for instance, is 14 commits ahead, 54 commits behind leapcode.

I think this is the main problem. This shouldn't happen, ever.

i agree that this is sub-optimal. i like your proposal of PRs from dedicated (feature-)branches and a special branch that we use for non-merged PRs.

regarding the platform git metrics i see from 

--- git/leap_platform ‹develop› » git log --no-merges --graph --pretty=format:'%Cred%h %aN%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset' --abbrev-commit --date=relative develop..pixelated_develop

that we have 20 commits that differ between our and LEAPs develop branch.

bwagnerr commented 8 years ago

If we didn't have the forks, it would have taken 3+ months for us to see shyba's improvements in soledad code (PR still pending, we still wouldn't have the mail leaks fix in leap mail and soledad (PR is still pending), I feel we definitely still need the forks.

Talking to meskio and kaliy here in SP, they also feel more comfortable with us having the forks, because that some of the pressure for them to accept the PRs and they can review them properly, according to their workflow and accept it when they feel confident with it.

Now about our workflow, there's a reason we use the develop branch and that was made evident a short time ago, we had some people use branches for a while and what happened is what I wouldn't like to see in our forks.

We had all the leap libraries with at least 10 commits ahead of leap and no urgency to PR them. If we do everything in develop, we create a PR for a library and we don't do anything else in that library until we get the PR merged or invalidated, that creates a sense or urgency and helps us not to leave commits behind in our fork. Also, if we create multiple PRs at the same time for a library, it is very probable that the maintainer won't be able to cope with the volume and things might get even slower in the end.

deniscostadsc commented 8 years ago

If we didn't have the forks, it would have taken 3+ months for us to see shyba's improvements in soledad code (PR still pending, we still wouldn't have the mail leaks fix in leap mail and soledad (PR is still pending), I feel we definitely still need the forks

As far as I understand from that conversation with Kali and Meskio, they are willing to release more often with we want. I truly believe we can't count on this, but if it's going to help us, why not ask them to release more often?

Talking to meskio and kaliy here in SP, they also feel more comfortable with us having the forks, because that some of the pressure for them to accept the PRs and they can review them properly, according to their workflow and accept it when they feel confident with it.

Some PRs take time to be reviewed and merged, we can't change that. And to be honest I really prefer wait for 3 months and have a pair reviewed code running on my machine than running some code that me only wrote.

Now about our workflow, there's a reason we use the develop branch and that was made evident a short time ago, we had some people use branches for a while and what happened is what I wouldn't like to see in our forks.

Could you please tell me what exactly was made evident a short time ago. That is not evident to me. Even if a problem happened with branch/PR approach, I would like to discuss that problem. I'm quite sure the problem isn't having forks or branches, the problem is how we deal with that.

We had all the leap libraries with at least 10 commits ahead of leap and no urgency to PR them. If we do everything in develop, we create a PR for a library and we don't do anything else in that library until we get the PR merged or invalidated, that creates a sense or urgency and helps us not to leave commits behind in our fork. Also, if we create multiple PRs at the same time for a library, it is very probable that the maintainer won't be able to cope with the volume and things might get even slower in the end.

I totally disagree with this point. One of the reasons of having forks is to have our changes faster. And then we start use a process that we have to wait for one PR at time be accepted. If I find a bug while waiting for a PR to be reviewed I have to wait until the PR merge to start to work on that bug. This is for sure going to slow us down. And probably, at least me, we are going to put that bug in a place to be forgotten.

I'm personally against to run "unofficial" code. From my perspective to have unmerged things on our main branch is wrong. But we have a very particular situation, and we probably have to do that in order to be able to work. But I think we need a better approach for that.

:heart:

bwagnerr commented 8 years ago

@deniscostadsc I agree that one is not necessarily related to other in some of the cases.

Explaining a little bit better, we started using different branches on soledad some time ago, because we needed to make different changes in a faster way. We still have 3 different changes in soledad that weren't merged, we gave up recently and rolled back two of those changes that didn't make sense anymore and created a new PR for the third, that was lost in the way before, we are still trying to catch up with leaving things up-to-date.

The only reason for the fork was to use the changes we did but were yet accepted, if we stop doing that, we don't need the forks

deniscostadsc commented 8 years ago

Explaining a little bit better, we started using different branches on soledad some time ago, because we needed to make different changes in a faster way. We still have 3 different changes in soledad that weren't merged, we gave up recently and rolled back two of those changes that didn't make sense anymore and created a new PR for the third, that was lost in the way before, we are still trying to catch up with leaving things up-to-date.

I'm sure this is a problem. But I don't think that having these never-merged PRs on our develop branch would solve the problem.

The problem I see is that LEAP release cycle is slow comparing to our development pace. The current approach solves the problem but bring a two problem:

That said I have three questions:

:heart:

[1] - https://github.com/pixelated/pixelated-user-agent/issues/687#issuecomment-218463494 [2] - https://github.com/pixelated/pixelated-user-agent/issues/687#issuecomment-218522370

deniscostadsc commented 8 years ago

I was looking for difference form our repos to LEAP's repos and I got this:

leap_platform (develop branch) This branch is 97 commits ahead of leapcode:develop. keymanager (develop branch) This branch is 2 commits ahead of leapcode:develop. soledad (develop branch) This branch is 14 commits ahead of leapcode:develop. leap_pycommon (develop branch) This branch is 1 commit ahead of leapcode:develop. leap_mx (develop branch) This branch is 4 commits ahead, 11 commits behind leapcode:develop. puppet_couchdb (master branch) This branch is 3 commits ahead, 4 commits behind leapcode:master. leap_web (develop branch) This branch is 93 commits behind leapcode:develop.

I think that we can get in a point that pixelated doesn't work with the official libs.

caiocarrara commented 8 years ago

Hi guys, sorry for answering it so late although being me whom started this talk offline. I first saw that we had a problem when I was dealing with soledad and trying to sync both repositories, upstream and our fork. There was a PR opened (Shyba refactoring) with more code than the proper one and probably it'll never be accepted. This is a problem because we're opening PR from our main branch.

Up to now what I could understand it's a choice because we need to solve a Leap slowness to accept our changes on some projects. It's not a real solution and Denis has already talked about:

Commiting changes on main branch and opening PR stop us to commit on this branch until this first PR be accepted (otherwise we're going to mess the changes on PR). Commit on the main branch and wait until PR be accepted is a kinda dangerous fake speed, because we're going to change our code once a time (until PR be accepted) and we're going to use code that is not official.

As official code I mean that code that was reviewed and approved by the core mainteiners of project which we use. Right now we're using code that was not checked through the Leap approval pipeline (whatever pipeline) and code that we're not sure will be approved anytime. On this way we're going to create our own project forked from Leap and it's OK if we decide it's OK. If we clearly decide for this way worth to mention that we will have to manage this "pipeline": let's put the projects on our build pipeline, do the CI (including new code comming from upstream), merge the conflicts, do adaptations to our needs, release our own package. So we can be sure we're doing the right job.

Right now, after looking the comments on this thread and thinking as a newcomer on the project, I think we should be using the official releases of each Leap project published on PyPI (I don't know if leap_platform is also published in any repository like PyPI). Getting those packages we're going to respect Leap QA process and keeping projects uncoupled as they should be. It can raise questions like: how we deal when we need a new feature on Leap projects, a bugfix or things like that? Again, my newcomer vision lead to a straightforward answer: create a fork from Leap project, submit a PR, wait for it be accepted and a new release. If it's a critical bugfix that need to be merged as soon as possible, I'm sure that Leap folks will put the proper attention on it.

I think in the past some decisions was made because the nature of relationship between Pixelated and Leap. It changed and now we have some good channel to talk with them and try to improve things like release more often. They're already using this closer relationship to suggest us new ways to distribute Pixelated.

bwagnerr commented 8 years ago

As official code I mean that code that was reviewed and approved by the core mainteiners of project which we use. Right now we're using code that was not checked through the Leap approval pipeline (whatever pipeline) and code that we're not sure will be approved anytime. On this way we're going to create our own project forked from Leap and it's OK if we decide it's OK. If we clearly decide for this way worth to mention that we will have to manage this "pipeline": let's put the projects on our build pipeline, do the CI (including new code comming from upstream), merge the conflicts, do adaptations to our needs, release our own package. So we can be sure we're doing the right job.

We do it right by those standards, they are in our build pipeline, we do have our own packages, but we decided always to follow mainstream instead of diverging for any reason.

There was a PR opened (Shyba refactoring) with more code than the proper one and probably it'll never be accepted. This is a problem because we're opening PR from our main branch.

As Denis pointed out deservedly in a comment of mine above, one thing does not imply the other. That commit being accepted or not (it will, but the changes were controversial and took time) have nothing to do with doing it on the main branch or not. Also incidentally, it was one of the few PRs we did on a feature branch and didn't use it for months until recently.

Right now, after looking the comments on this thread and thinking as a newcomer on the project, I think we should be using the official releases of each Leap project published on PyPI (I don't know if leap_platform is also published in any repository like PyPI)

This is what we did before, but I don't believe we are at a point to do this yet, they want to release faster but they are not releasing faster now. I would be comfortable with this after I see Leap releases every two weeks or so consistently for some months. Leap platform is another beast, it is not python and is not on pypi.

deniscostadsc commented 8 years ago

@bwagnerr isn't today your day off?! Go to rest!

bwagnerr commented 8 years ago

Reinforcing what I said on my day on :)

thaissiqueira commented 8 years ago

We discussed a lot and we realized that most of the problem we are facing with our forks is just a symptom of how we are using LEAP packages.

So we think that we are not going to kill our forks now, but this is going to happen as soon as we achieve best usage LEAP packages. Doing this we still need to wait the PRs to be accepted to have our code on the packages.

The steps to that are:

  1. Start to use our deb packages separately, not inside pixelate-user-agent deb package (We already need this)
  2. Share knowledge to LEAP start to build nightly packages.
  3. Start to use LEAP nightly packages
  4. We can bury our forks :D

Besides that we think that we still have a problem with the visibility of our PRs. We still need to think how deal with that.

Now we already can:

that).

What do you think? Can we go ahead with this plan? 🚀

cz8s commented 8 years ago

if send a pr to leap, will we have to wait until the pr is accepted before we can use the code? I don't think this would be a good thing. I would prefer to have our own forks and have our changes in there as soon as possible. We are working differently from leap and we have to acknowoledge that. It doesn't really matter if we use packages or not. It just depends on where the code comes from. And I think it makes sense to use our forks to keep everything running and to minimize waiting times. I don't think leap will ever be fast enough for us merging our prs. We are more people with more time. And waiting for them will slow us down.

All we need to do is to keep in sync with them. And this can be done with some git foo: This could be done in our develop branch regularly (after a pr got merged)

git fetch leap 
git rebase --soft leap/develop
git add .
git commit -m 'our changes that are not merged yet'

We also snhould have branches for our prs:

git commit -m 'our change' 
git checkout -b 'pr branch'
git cherry-pick id_of_commits_for_pr
do pr

img_20160602_140722

deniscostadsc commented 8 years ago

if send a pr to leap, will we have to wait until the pr is accepted before we can use the code? I don't think this would be a good thing. I would prefer to have our own forks and have our changes in there as soon as possible.

I'm not sure we need those PRs been accepted that quickly. I think is OK to wait the PRs be merged. Doing this we have a validation that the code we wrote to LEAP is really OK, including for them.

If they start to package nightly packages, we'll have our fork just to have the PRs faster. If this is a problem, I think this is the only point that this approach won't solve, right?

We are working differently from leap and we have to acknowoledge that.

This is something we discussed. We use LEAP libraries or we use our libraries that are based on the LEAP's? We have to be sure about that.

caiocarrara commented 8 years ago

The point raised by @deniscostadsc is important:

We use LEAP libraries or we use our libraries that are based on the LEAP's?

When we previous talk about this topic here we considered that we're using LEAP libraries and we're not willing to maintain other projects based on theirs (are we?).

@cz8s, from your draw, when the merge (in sync) between LEAP and Pix develop branches be done we're going to have two commits with the same change. The first one with the change and the last one form merge. I don't think this is correct. Today, if I'm not wrong, we're doing a process kinda of that and having to forcibly push merges that we do. Forcing pushes is clearly a "process smell".

I agree with @deniscostadsc that maybe we do not need our PRs be accepted so quickly. In the past our project probably had reasons to do so many changes on LEAP libs and get these changes available quickly. Looking for our current project stage I'm not sure we're going to need this "hurry". If we need too much something I believe that today we have a better relationship with LEAP and we can talk with them as we're doing now during our calls. Besides that, they already told us that is a plan release often.

Also, a question came here to me. Why do we need go "so fast" that we cannot wait for a PR on LEAP? Last time here in SP LEAP share with us some doubts to release often, probably could be a good idea try to help them release more, showing the advantages, how to automate things, etc. I think this is a nice opportunity that open source give us: if we see that we can collaborate to improve a project we can do.

I think I'm digressing to much right now, haha.

cz8s commented 8 years ago

@cacarrara : if we use the git commands I proposed, we only have one commit with our changes on top of the original leap repo at any given point in time. And would love to do some cd here and I don't think this will be possible if we have to wait forleap about our pr

cz8s commented 8 years ago

Okay. My conclusion:

There are pro/con for both aproaches. What we have to ensure is:

If we are not afraid that we get blocked if we wait until upstream merged, this would ensure that we don't diverge because we don't need forks.

If we are afraid that we get blocked if we wait until upstream merged, we need forks with our changes in it. Then we need a way to manage our changes withou diverging from upstream. This can be done in git but everyone needs to know how to do it and its a strange workflow. Are we confident we want to do this?

cz8s commented 8 years ago

descision made: we will have a fork of the leap libs but will not commit to the develop branch. We will do PRs to leap and wait until they get merged. We will then merge the changes from leap into our develop branch.

By doing this, our code will not diverge with the leap code, but we still have our own packages and can commit to our fork if we are in firefighting mode and leap is on vacation :)

deniscostadsc commented 8 years ago

Champions for each repository:

cz8s commented 8 years ago

leap_platform and keymanager are tracking the leap branches now

cz8s commented 8 years ago

soledad has commits from naval that is not on lea/develop.

Your branch and 'origin/develop' have diverged,
and have 6 and 28 different commits each, respectively.
bwagnerr commented 8 years ago

Hey @cz8s I updated soledad with the latest commit from kaliy (made today), there are no pending naval commits and now we are almost in sync with soledad, the only pending issue is that there's one of shyba's commits that wasn't included in the merge, I'll talk to them to figure this one out.

cz8s commented 8 years ago

can we move that commit into an extra branch? Or do we need it? I would like to do a git reset --soft leap/develop in our branch to ensure there is no difference

varac commented 8 years ago

For puppet_couchdb, the only commit that differs from LEAPs master branch is the inclusion of the SNAP build icon that is special to our fork, so i'd leave it there if noone rejects:

--- other-modules/couchdb ‹master› » git branch-diff remotes/leap_github/master..remotes/pixelated/master 
* 3c8deca - add build-status badge from snap (7 months ago)
--- other-modules/couchdb ‹master› » git show 3c8deca
commit 3c8deca6cdcc435265473f042991e78c5248a570
Author: Christoph <chris@inferno.nadir.org>
Date:   Thu Nov 26 22:58:58 2015

    add build-status badge from snap

diff --git a/README.md b/README.md
index 096221a..e4a0157 100644
--- a/README.md
+++ b/README.md
@@ -1,5 +1,7 @@
 # Couchdb Puppet module

+[![Build Status](https://snap-ci.com/pixelated/puppet_couchdb/branch/master/build_image)](https://snap-ci.com/pixelated/puppet_couchdb/branch/master)
+
 This module is based on the one from Camptocamp_.

 .. _Camptocamp: http://www.camptocamp.com/
cz8s commented 8 years ago

I would prefer to not have the badge and have the branches exactly the same.

bwagnerr commented 8 years ago

@cz8s @varac me too

bwagnerr commented 8 years ago

Just synchronized soledad again, now we are exactly on the same commit as leap :)

deniscostadsc commented 8 years ago

@cz8s @varac

Are these commit important? https://github.com/leapcode/leap_mx/compare/develop...pixelated:develop

deniscostadsc commented 8 years ago

I did the reset on leap_mx. If those commits are important I can undo this.

cz8s commented 8 years ago

Maybe put them in a dedicated branch?

deniscostadsc commented 8 years ago

@cz8s

I didn't find a better name.

https://github.com/pixelated/leap_mx/tree/old_develop

cz8s commented 8 years ago

:)

deniscostadsc commented 8 years ago

Can we close this now?

deniscostadsc commented 8 years ago

@tuliocasagrande raised a conversation about pep8 on LEAP repositories, maybe we should talk about a pep8 rules and checking on their side.

cz8s commented 8 years ago

we will talk about pep8 on our next call