Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

Try Vanilla Forums Cloud product
Vanilla 2.6.1 is here with critical security patches. One of them has been publicly disclosed.

Pull requests & you

LincLinc Director of DevelopmentDetroit Vanilla Staff
edited April 17 in Development

This is a quick overview of pull requests for Vanilla and how the community can help get releases out the door faster.

The very basics:

  • Fork the repository. (You won't be able to push to our repos directly.)
  • Pull request against master branch (FROM the branch on your fork TO our copy's master)
  • Verify you have a CLEAN pull request without extra garbage in the change set.
  • Write a great, release-notes-worthy title for the pull request.
  • Write a well-considered description that details the changes being made & your rationale.
  • Treat GitHub as an extension of the community: be friendly, say hello to new folks, etc.
  • Mention what you've done to test it already, and what others should be looking at in their tests.

Beyond the basics:

Community evaluation of pull requests would be tremendously helpful. I realize you can't merge PRs, but there's a lot of work that happens before we get that far. And, you'll learn a tremendous amount by jumping in.

If you don't have a deep Vanilla understanding, you can look at the superficial: does it meet our coding standards and naming conventions? Is it a decent idea? Is it free of syntax errors? Has the user signed our contributors' agreement?

If you've been around a while and have been building some serious addons, go further: Is this a safe & sane idea? Can you test it in multiple scenarios to see if it causes a regression or unintended consequences? Is this a Vanilla-like thing to do? Is there already a simpler way to accomplish what the author wants without their change? (e.g.: if they say they need a hook to do X, do you know an existing hook or better way to do it?)

When you evaluate a PR, say so in the comments. We're not looking for cheerleader comments like "yay good idea", but rather "I agree this is an issue/good improvement because I've seen X or understand what they're trying to accomplish. I tested this PR by doing exactly W, Y, and Z and these were my results".

The backlog is a bit long right now, but we're gonna get 'er done. :) Keep pullin'!

BleistivtR_JhgtonightItsVizionTv

Comments

  • hgtonighthgtonight ∞ · New Moderator

    If you want to be notified of PRs and have a github account, click the Watch icon and select Watching on the main Vanilla repository here: https://github.com/vanilla/vanilla

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

  • R_JR_J Cheerleader & Troubleshooter Munich Moderator
    edited September 2014

    @Linc said:

    • Features & significant changes: Pull request against stage branch.
    • Hotfixes & tiny, safe changes: Pull request against master branch.

    I only have a 2.1 installation by now. Do the two points above implicit that

    a) I have to install master and stage branch
    b) or are those identical versions with simply two different names
    c) I first have to check if problem I found in 2.1 still prevail in master/stage
    d) I have to fix and test against master/stage

    I have a fix for an issue and normally would have made a Pull Request against the 2.1 file, but it has been changed in 2.2 and so I think the answer to my questions above are a) yes b) ??? c) yes and d) also yes, but could you (or anyone else) please confirm that (and answer b) for me)? I wouldn't like to install something if I didn't have to.

    Edit: found an anwer for b) https://github.com/vanilla/vanilla/compare/stage
    They are not the same, so I need two installations

  • LincLinc Director of Development Detroit Vanilla Staff
    edited September 2014

    @R_J That was general Vanilla development procedure. If you have a PR for a 2.1 patch, by all means, pull against 2.1. It's very helpful if you also include notes like "this is already fixed in master" or "this should go to master" or "this is still a problem in master but this won't fix it" - but not required, certainly.

    Stage is always slightly ahead of master; it's where we land new features before they go to master. Master is in an always-ready-to-deploy state. Stage is in a "probably OK but you'll wanna integration test it more first" state.

  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    Installation already done - both stage and master! :)

    And that was a good thing to do because the issue I was looking at has been already fixed for 2.2 (reported that on GitHub a second before).

  • LincLinc Director of Development Detroit Vanilla Staff
    edited September 2014

    Pro tip: I do 1 install each for release branches (2.0 and 2.1), and 1 for master/stage. My installs are directly linked to my git repos so I can toggle as I need to by just checking out a branch. It's extremely rare you would run into a breaking change switching between the two (or one that requires more than utility/update).

    I don't recommend checking out between 2.0/2.1/master on your installs because that will get messy. I have 3 repo copies of Vanilla.

  • LincLinc Director of Development Detroit Vanilla Staff

    Also, remember that every fix in master is a fix for the next release :) The more we deal with via master/stage, the better 2.3 will be.

  • chanhchanh OngETC.com - CMS Researcher ✭✭

    This is helpful!
    Thanks

  • peregrineperegrine MVP
    edited September 2014

    Also, remember that every fix in master is a fix for the next release

    can you explain more fully.
    is it something you plan to do in the future.

    e.g. will 2.3 be using upgraded jQuery that master now uses? will ALL bug fixes that have been implemented in master now, be in 2.3

    I may not provide the completed solution you might desire, but I do try to provide honest suggestions to help you solve your issue.

  • JasonBarnabeJasonBarnabe Cynical Salamander ✭✭

    Thanks for the info. It's a bit disheartening to go through the trouble of submitting pull requests only to have them untouched, so this is encouraging.

    Does everything also apply to the addons repository?

  • @JasonBarnabe there is no guarantee that a pull request will get a pass. Not everything belongs in the core an some issue are more important than others.

    grep is your friend.

  • LincLinc Director of Development Detroit Vanilla Staff
    edited September 2014

    @JasonBarnabe said:
    Thanks for the info. It's a bit disheartening to go through the trouble of submitting pull requests only to have them untouched, so this is encouraging.

    I fully commiserate, and apologize for the lapse. Obviously not all will make it into core, but we should communicate that ASAP.

    @JasonBarnabe said:
    Does everything also apply to the addons repository?

    It does. I plan to fork Addons to have a 2.3 branch this time as well. We haven't done release branches there in the past, and it's gotten us into trouble.

    R_J
  • LincLinc Director of Development Detroit Vanilla Staff

    @peregrine said:
    can you explain more fully. is it something you plan to do in the future.

    Great question.

    Whenever we start a new release cycle, we copy the current state of master as the seed of the next release. This was less obvious with 2.1 because we forked it so far in the past before it finally made it to release. It took more than a year from when it split from master to when we were "official". That's really bad; 2.3 should be closer to 3 months, I hope.

    The idea is that we have big stuff coming into master constantly that would destabilize our release. So we take a snapshot (fork) and from then on just add smaller patches until we get the official release. master keeps chugging along, never noticing it had a baby. :)

  • LincLinc Director of Development Detroit Vanilla Staff

    Excepting 2 I commented on for further assistance, I've caught up all pull requests to 2014.

    hgtonightJasonBarnabe
  • LincLinc Director of Development Detroit Vanilla Staff
    edited December 2014

    I made a new badge for pull requests that leave me particularly chuffed: http://vanillaforums.org/badge/high-five

  • LincLinc Director of Development Detroit Vanilla Staff
  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    Git is still very hard for me to understand :(
    When I want to make a pull request that affects only one file, I simply do it over the GitHub web interface by directly editing the file. That already creates a pull request.

    In what way is that GitHub Flow different?

  • hgtonighthgtonight ∞ · New Moderator

    @R_J said:
    Git is still very hard for me to understand :(
    When I want to make a pull request that affects only one file, I simply do it over the GitHub web interface by directly editing the file. That already creates a pull request.

    In what way is that GitHub Flow different?

    Looks like it gives you a chance to name the branch

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

  • chanhchanh OngETC.com - CMS Researcher ✭✭

    That is nice addition!
    Thanks for the info.

  • LincLinc Director of Development Detroit Vanilla Staff

    I have edited the OP to only open pull requests against master branch.

  • LincLinc Director of Development Detroit Vanilla Staff

    Bumping this up. We do not currently have a PR backlog, and our issue backlog is considerably trimmed.

    ItsVizionTvhgtonight
  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    @Linc said:
    I have edited the OP to only open pull requests against master branch.

    I did so and now I'm trying to backport all those little fixes I've made against the 2.3 release. This is really annoying...

    I expect that there will be a point in time, after 2.3 is released and current, when all the PRs against the master branch will make it into the second to next release (2.5?), but not into the next release (2.4).
    From this time on - to my understanding - all commits to master will not make it to the upcoming version (2.4) and thus must be made against two branches.

    That's why I think it is a bad advice to generally commit PRs against master. If there is a branch for the next release, a PR should be made for both branches, otherwise that fix wouldn't be available for OS users in the near future.

    That's what I learned today when I took a closer look at 2.3RC1 and found that many known issues with already existing fixes still persist. And that's why I'm going through the list of my own PRs and try to backport them.

  • LincLinc Director of Development Detroit Vanilla Staff
    edited October 2016

    That's not accurate. I fork from master at the time I release a beta version of a release. I do not fork for beta until after a final release. So if you add to master while 2.3 is in the pipe, everything you add will be in 2.4.

Sign In or Register to comment.