charmhelpers migration to github

James Hebden james.hebden at canonical.com
Thu Sep 21 10:12:26 UTC 2017


Just lending my support for Dmitrii's guidance here.
Clean history is important, especially to those reviewing changes - but
having a clean history isn't as simple as having a single commit in
place when merging code from a private branch into upstream - it's
more important to isolate logical changes into individual branches and
MP those regularly and without remorse.
It's a fairly comfortable and obvious distinction - but an important
one I think when working with MPs.

Reviewing these two commits in one MP -
"Updated bundled dependencies"
"Corrected PEP8 errors"

Or one commit
"Updated bundled dependencies and corrected PEP8 errors"

...both are going to be a nightmare for the reviewer, especially given
the GitHub UI's inability to sensibly show changes to a single file when
a lot of files have changed, especially over multiple commits. Keeping
MPs short, sweet and logical will make everyone's lives easier when
working with MPs.

So, If I'm just agreeing with everyone, why am I replying? Well,
primarily just to point everyone at
https://github.com/juju/juju/blob/develop/CONTRIBUTING.md#workflow

The existing Juju docs seem to get it right and meet everyone's
expectation here. Seems like a good candidate for improving the current
charmhelpers contributing documentation.

Also, thanks for all the hard work to make this happen! 

On Thu, Sep 21, 2017 at 12:35:56PM +0300, Dmitrii Shcherbakov wrote:
> I think that following the clean history approach is generally good as it
> makes your life easier during debugging and commit message grepping.
> 
> Linus' point about
> 
> https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html
> "I want clean history, but that really means (a) clean and (b) history."
> 
> having both history and keeping it clean is something that we should
> consider but not enforce too much (subjectively) to avoid making it too
> hard to commit changes. In the end, this transition to github is about
> making it easier to contribute, not requiring a person to read a 100-page
> manual on how to annotate and prepare a commit to push a one-liner.
> 
> Given that charm-helpers changes are generally small, I think squashing is
> OK even using github's mechanism.
> 
> For large commits, on the other hand, it makes sense to recreate a pull
> request (close a PR, update and push to a fork, create a new PR) or update
> an existing PR by doing a complex rebase + force push. When there are
> several logical changes per commit it is quite important to keep them
> separate and squashing everything into a single change is essentially
> hiding history.
> 
> An analogy would be file compression: yes, you can squeeze files to a
> single compressed blob and make it a bit smaller but then you have to waste
> CPU cycles to decode it. In the same way, when you are trying to quickly
> grep through a git log you don't want to waste brain cycles on decoding
> unstructured information.
> 
> Best Regards,
> Dmitrii Shcherbakov
> 
> Field Software Engineer
> IRC (freenode): Dmitrii-Sh
> 
> On Thu, Sep 21, 2017 at 12:02 PM, Merlijn Sebrechts <
> merlijn.sebrechts at gmail.com> wrote:
> 
> > It depends on what you want to optimize the development workflow for. I
> > think we need to optimize for easiness because a lot of contributors will
> > be ops people who generally have a lot less experience with git and github.
> > I for example have rebased once in my life, and this was only possible with
> > Alex walking me through the process.
> >
> > *"Fork to private + PR + dirty fix commits" *is an easy workflow that a
> > lot of people are familiar with and that works well with Github. If you
> > want a cleaner commit history, you can always rebase or squash the PR
> > during merge using the Github UI: https://pasteboard.co/GLmTHnf.png.
> >
> > It's not ideal but it's a small price to pay for more contributors..
> >
> >
> >
> >
> >
> >
> > 2017-09-20 22:10 GMT+02:00 Alex Kavanagh <alex.kavanagh at canonical.com>:
> >
> >> There's some interesting ideas in there, Dmitrii. Whatever workflow we
> >> end up with needs to be consistent with the other workflow on the juju
> >> namespace on github.com, which I'm guessing is a simple fork to private
> >> + PR.
> >>
> >> I've used squashed commits on projects in the past, and they do lead to a
> >> cleaner git history, which is nice; as you say, it's a bit like gerrit.
> >> Unfortunately, it's not gerrit, so it's difficult (as the bugs indicate) to
> >> get it to work like gerrit, if you want to preserve the PR history, yet
> >> keep clean commits.  Once it gets to a PR I think you're pretty much stuck
> >> with the "GitHub way".
> >>
> >> Cheers
> >> Alex.
> >>
> >> On Wed, Sep 20, 2017 at 8:33 PM, Dmitrii Shcherbakov <
> >> dmitrii.shcherbakov at canonical.com> wrote:
> >>
> >>> > I'm guessing that the development workflow will be to fork the repo,
> >>> and do PRs from your own github version?
> >>>
> >>> In short, yes.
> >>>
> >>> 1. fork;
> >>> 2. clone locally;
> >>> 3. set up 2 remotes (1 for rebasing to upstream, 1 for pushing);
> >>> 4. create a branch, commit and push to your fork;
> >>> 5. create a PR.
> >>>
> >>> > I also guess that the contributing guide will need updating (it talks
> >>> about bzr).
> >>>
> >>> I would also suggest PR workflow-related updates to that doc given that
> >>> one cannot
> >>>
> >>> git rebase -i HEAD~<n> # amend, squash, add new commits etc.
> >>> and
> >>> git push # to a forked repo
> >>>
> >>> without doing a force push to update a pull request. In my view, it's
> >>> good to keep the commit history clean instead of adding several commits on
> >>> top without squashing them. Otherwise it quickly turns into:
> >>>
> >>> "an original commit message to make charm-helpers great
> >>> fixup commit to avoid a typo
> >>> hotfix to the fixup
> >>> final fix - will not happen again"
> >>> * closed a PR as a huge rebase is needed
> >>>
> >>> I would propose the following:
> >>>
> >>>
> >>>    - using "push -f" only for private branches used for pull requests
> >>>    https://help.github.com/articles/using-git-rebase-on-the-com
> >>>    mand-line/#pushing-rebased-code-to-github
> >>>    <https://help.github.com/articles/using-git-rebase-on-the-command-line/#pushing-rebased-code-to-github>
> >>>    - using git-pull-request: https://github.com/jd/git-pull-request
> >>>    which updates PRs with push -f.
> >>>    - following this workflow advice about branches: https://github.com/j
> >>>    d/git-pull-request#workflow-advice
> >>>    <https://github.com/jd/git-pull-request#workflow-advice>
> >>>
> >>> Rationale:
> >>>
> >>>    - https://blog.adamspiers.org/2015/03/24/why-and-how-to-correc
> >>>    tly-amend-github-pull-requests/
> >>>    <https://blog.adamspiers.org/2015/03/24/why-and-how-to-correctly-amend-github-pull-requests/>
> >>>    - https://softwareengineering.stackexchange.com/a/263172
> >>>    - https://blog.adamspiers.org/2017/08/16/squash-merging-and-ot
> >>>    her-problems-with-github/
> >>>    - https://www.mail-archive.com/dri-devel@lists.sourceforge.net
> >>>    /msg39091.html
> >>>
> >>>
> >>> More info in a gist.
> >>> https://gist.github.com/dshcherb/2c827ae945dc551da3681313294d6783
> >>>
> >>>
> >>> Coming from the kernel-type patch-sending process (
> >>> https://lwn.net/Articles/702177/, https://www.mail-archive.com/d
> >>> ri-devel at lists.sourceforge.net/msg39091.html) and gerrit (
> >>> https://www.mediawiki.org/wiki/Gerrit/Tutorial#Comparing_patch_sets) I
> >>> find github's approach with fixup commits a little strange but
> >>> force-pushing with precautions even to a PR branch is not a silver bullet
> >>> of course.
> >>>
> >>> https://github.com/isaacs/github/issues/997
> >>> https://github.com/isaacs/github/issues/999
> >>>
> >>> It would be great to hear some feedback on this topic so that we are not
> >>> doing anything random and have a common workflow.
> >>>
> >>> Thanks in advance!
> >>>
> >>> Best Regards,
> >>> Dmitrii Shcherbakov
> >>>
> >>> Field Software Engineer
> >>> IRC (freenode): Dmitrii-Sh
> >>>
> >>> On Wed, Sep 20, 2017 at 4:14 PM, Alex Kavanagh <
> >>> alex.kavanagh at canonical.com> wrote:
> >>>
> >>>> Great stuff; I can confirm that I'm in.  I'm guessing that the
> >>>> development workflow will be to fork the repo, and do PRs from your own
> >>>> github version?
> >>>>
> >>>> I also guess that the contributing guide will need updating (it talks
> >>>> about bzr).  I'm happy to do a PR for that if the workflow can be confirmed
> >>>> :)
> >>>>
> >>>> Cheers
> >>>> Alex.
> >>>>
> >>>>
> >>>> On Wed, Sep 20, 2017 at 12:59 PM, James Page <james.page at ubuntu.com>
> >>>> wrote:
> >>>>
> >>>>> If you're a part of the charmers team on Launchpad you should now
> >>>>> either have access to approve pull requests + merge or you should have an
> >>>>> invite to join the team that can do this :-)
> >>>>>
> >>>>> If you don't have one PM me on freenode IRC with your github username.
> >>>>>
> >>>>> On Wed, 20 Sep 2017 at 11:57 James Page <james.page at ubuntu.com> wrote:
> >>>>>
> >>>>>> Hi All
> >>>>>>
> >>>>>> Heres a bit of a status update on migration activity:
> >>>>>>
> >>>>>> Code history migration completed
> >>>>>> Travis CI enabled for unit testing and linting with Py 2.7 and 3.4
> >>>>>> Repo configured to not allow merges until Travis +1's
> >>>>>>
> >>>>>> TODO
> >>>>>>
> >>>>>> Make sure all members of the current team on launchpad are part of
> >>>>>> the charmhelpers team - that should be completed today
> >>>>>> Fixup charmhelpers sync tooling to work from github - this week
> >>>>>> (mainly used by OpenStack Charms team)
> >>>>>> Redirect lp:charm-helpers landings to github.com/juju/charm-helpers
> >>>>>>
> >>>>>> and the prize goes to Merlin for raising the first non-migration
> >>>>>> related pull request :-)
> >>>>>>
> >>>>>>
> >>>>>> On Tue, 19 Sep 2017 at 14:57 Bryan Quigley <
> >>>>>> bryan.quigley at canonical.com> wrote:
> >>>>>>
> >>>>>>> From other projects I've seen moved, I'd much prefer if the Code
> >>>>>>> section (and any other sections not planned on being using anymore) were
> >>>>>>> cleared out on LP and then disabled.
> >>>>>>>
> >>>>>>> Thanks!
> >>>>>>> Bryan
> >>>>>>>
> >>>>>>> On Tue, Sep 19, 2017 at 9:42 AM, Marco Ceppi <
> >>>>>>> marco.ceppi at canonical.com> wrote:
> >>>>>>>
> >>>>>>>> I've updated the launchpad description to highlight the change.
> >>>>>>>> Since there's bound to be processes still pointing at the lp branch, should
> >>>>>>>> we set it up as a mirror from git?
> >>>>>>>>
> >>>>>>>> On Tue, Sep 19, 2017 at 9:37 AM James Page <james.page at ubuntu.com>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> OK - step 1 completed; I've pushed fresh bzr->git migrated code to
> >>>>>>>>>
> >>>>>>>>>    https://github.com/juju/charm-helpers
> >>>>>>>>>
> >>>>>>>>> Please don't land any further changes into the bzr branch as we'll
> >>>>>>>>> need to diverge from this point forwards.
> >>>>>>>>>
> >>>>>>>>> I will land a commit in lp:charm-helpers to point lost souls to
> >>>>>>>>> the new github.com location as part of the migration.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Mon, 18 Sep 2017 at 14:15 Alex Kavanagh <
> >>>>>>>>> alex.kavanagh at canonical.com> wrote:
> >>>>>>>>>
> >>>>>>>>>> I'm a +1 on this too.  Let the good times roll.
> >>>>>>>>>>
> >>>>>>>>>> On Mon, Sep 18, 2017 at 11:22 AM, James Page <
> >>>>>>>>>> james.page at ubuntu.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> Resurrecting this thread; I think its a good time to push on
> >>>>>>>>>>> with this work - anyone have any objections to targeting this week to
> >>>>>>>>>>> complete the migration?
> >>>>>>>>>>>
> >>>>>>>>>>> On Fri, 21 Jul 2017 at 19:55 David Ames <
> >>>>>>>>>>> david.ames at canonical.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> On Fri, Jul 21, 2017 at 5:46 AM, James Page <
> >>>>>>>>>>>> james.page at ubuntu.com> wrote:
> >>>>>>>>>>>> > Hi All
> >>>>>>>>>>>> >
> >>>>>>>>>>>> > Managed to find some time to test the bzr->git migration
> >>>>>>>>>>>> more, including
> >>>>>>>>>>>> > some tidy of committers and other general hygiene.
> >>>>>>>>>>>> >
> >>>>>>>>>>>> >    https://github.com/juju/charm-helpers
> >>>>>>>>>>>> >
> >>>>>>>>>>>> > I think we're in a good position to plan for a switch - I
> >>>>>>>>>>>> appreciate there
> >>>>>>>>>>>> > are a number of open reviews against the bzr branch for
> >>>>>>>>>>>> charmhelpers so it
> >>>>>>>>>>>> > would be nice to get those landed where possible first.
> >>>>>>>>>>>> >
> >>>>>>>>>>>> > I can re-run the process at any time so we can pick when we
> >>>>>>>>>>>> want to actually
> >>>>>>>>>>>> > switch over.
> >>>>>>>>>>>> >
> >>>>>>>>>>>> > Once we have migrated, we can push forward on travis setup
> >>>>>>>>>>>> etc... so that we
> >>>>>>>>>>>> > can automatically test pull requests.
> >>>>>>>>>>>> >
> >>>>>>>>>>>> > Cheers
> >>>>>>>>>>>> >
> >>>>>>>>>>>> > James
> >>>>>>>>>>>>
> >>>>>>>>>>>> I landed two of Alex's MPs today which fix unit test failures
> >>>>>>>>>>>> that
> >>>>>>>>>>>> would need to get pulled in. Other than that, the road is clear
> >>>>>>>>>>>> from
> >>>>>>>>>>>> the OpenStack Charm team.
> >>>>>>>>>>>>
> >>>>>>>>>>>> --
> >>>>>>>>>>>> David Ames
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> --
> >>>>>>>>>>> Juju mailing list
> >>>>>>>>>>> Juju at lists.ubuntu.com
> >>>>>>>>>>> Modify settings or unsubscribe at:
> >>>>>>>>>>> https://lists.ubuntu.com/mailman/listinfo/juju
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> Alex Kavanagh - Software Engineer
> >>>>>>>>>> Cloud Dev Ops - Solutions & Product Engineering - Canonical Ltd
> >>>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> Juju mailing list
> >>>>>>>>> Juju at lists.ubuntu.com
> >>>>>>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> >>>>>>>>> an/listinfo/juju
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> Juju mailing list
> >>>>>>>> Juju at lists.ubuntu.com
> >>>>>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> >>>>>>>> an/listinfo/juju
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>> --
> >>>>> Juju mailing list
> >>>>> Juju at lists.ubuntu.com
> >>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> >>>>> an/listinfo/juju
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>> --
> >>>> Alex Kavanagh - Software Engineer
> >>>> Cloud Dev Ops - Solutions & Product Engineering - Canonical Ltd
> >>>>
> >>>> --
> >>>> Juju mailing list
> >>>> Juju at lists.ubuntu.com
> >>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> >>>> an/listinfo/juju
> >>>>
> >>>>
> >>>
> >>
> >>
> >> --
> >> Alex Kavanagh - Software Engineer
> >> Cloud Dev Ops - Solutions & Product Engineering - Canonical Ltd
> >>
> >> --
> >> Juju mailing list
> >> Juju at lists.ubuntu.com
> >> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> >> an/listinfo/juju
> >>
> >>
> >

> -- 
> Juju mailing list
> Juju at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju


-- 
James Hebden
Cloud Reliability Engineer
BootStack Squad @ Canonical Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/juju/attachments/20170921/fe760a52/attachment.sig>


More information about the Juju mailing list