owncloud / gallery

:sunrise: Gallery app for ownCloud, which includes previews for all supported media files
GNU Affero General Public License v3.0
88 stars 65 forks source link

Replacing the current Gallery and aligning the branching model #209

Closed jancborchardt closed 9 years ago

jancborchardt commented 9 years ago

Currently there’s 2 big organizational things we need to do with Gallery+

1. Make it replace the current Gallery app

When we rename the repos, there will be build issues for older ownCloud versions. @DeepDiver1975 suggested to issue a pull request to the gallery repo with the current master code of this galleryplus repo. @oparoz then your Gallery+ app would become the new master, and we can continue work there under the name »Gallery« as we wanted before.

2. Align the branching model with core, like other apps do, so it’s less confusing

Stuff like

Now available on ultimatedev for core stable8.1

is just majorly confusing, as are the 13, 14, 15 etc for milestones. This is annoying for any newcomers as well as for more seasoned ownCloud devs. We are used to the master and stable8.1, stable8, stable7 etc branches which are used in core as well as in other apps. We just need to do the same for Galleryplus.

@oparoz @DeepDiver1975 @setnes @Spacefish

oparoz commented 9 years ago

Branches

Well, the branches are not that different, except that I prefer to use master to target the production channel , currently still at 8.0, so that all can benefit. Then there is dev to test what's coming next (8.1/8.2), the stableX branches (like on oC) and finally the ultimate branches which are created purely as a convenience to casual contributors and myself who don't want to have to lose some of the current features just to be able to test a PR or two.

If it's a requirement to commit to master and target the unstable version of ownCloud, then I'll create a PR using the dev branch from here. From my point of view doing so benefits core devs, but makes it harder for newcomers to contribute to the project because 1) they need to setup an unstable core env 2) they might not be able to easily determine if problems they see comes from their changes or something broken in core

Maybe I'll just accept PR made against other branches while I can.

To me, making commits for 8.2 right now, when I'm on 8.0 doesn't make much sense, unless I need to make some big, breaking changes because of some new features in core, but that's what dev is for from my pov.

At the end of the day, I've seen both approaches. I hope others will voice their opinion, but I'll go with what ownCloud wants. My main fear is that older versions of oC won't see much action, but if it doesn't work for me and others, I can always create my own fork for older versions and maybe smaller contributors can use that.

Milestones

There are a couple of major differences between an app and core:

So we can't use a 8.1-current milestone in an app.

My idea was to just have app release milestones (12,13, etc.) and use labels to indicate which core release an issue targets.

In PRs the labels indicate on which branch the PR was made against.

jancborchardt commented 9 years ago

Well, the branches are not that different, except that I prefer to use master to target the production channel , currently still at 8.0, so that all can benefit. Then there is dev to test what's coming next (8.1/8.2), the stableX branches (like on oC)

Well – then just switch it around. Currently you use:

Switch that to:

Backports can always be easily made by cherry-picking.

and finally the ultimate branches which are created purely as a convenience to casual contributors and myself who don't want to have to lose some of the current features just to be able to test a PR or two.

Pull requests can directly be checked out as branches, so that is already covered.

oparoz commented 9 years ago

Actually

My problem is that I don't care at all about 8.2 right now. I want to focus on 8.1 so that when it reaches the production channel, the app is ready and stable.

What will probably happen is we'll get an official Gallery app, following core's rules:

and an approved fork, containing features and bug fixes for the current release.

Pull requests can directly be checked out as branches, so that is already covered.

Yes, but that's one PR at a time if I'm not mistaken and unless they're regularly rebased, the base might be old.

deMattin commented 9 years ago

I agree with @jancborchardt that it is actual confusing with "ultimate", "dev", "ultimatedev" and so on. I had to read some comments to know, which one I should use on my OC 8.0 install.

Do I understand it right, that there's only the need of a stable 8.x branch, if there are features of core presupposed? And backports are only neccessary for security fixes, I think.

So stable are the "released" and tested versions (stable7, stable8 and in this case stable8.1 because of core requrements). And for development? How is this to be named? Could be "master8" (or "master8.1" because of the dependency on core 8.1) And for single feature pre-tests it could be "master8.1-nicenewfeaturepretest".

As an example, if the dependency on 8.1 would't have been given, there we would only have a "master8" and maybe some "master8-..." branches for development and a stable8 for "normal user", who don't want to test and just use OC8.x.

Did I get it right? This way I understand it now and I would like it, because it seems to be intuitive.

Edit: Naming a branch only "master" is irritating, because you don't know, which core version it depends on.

jancborchardt commented 9 years ago

Pull requests can directly be checked out as branches, so that is already covered.

Yes, but that's one PR at a time if I'm not mistaken and unless they're regularly rebased, the base might be old.

@oparoz that’s kind of the point of testing. ;) Otherwise you do not know which PR new issues you encounter were introduced by.

Edit: Naming a branch only "master" is irritating, because you don't know, which core version it depends on.

@deMattin simple – it would be core master.

oparoz commented 9 years ago

I guess what was missing was some explanation in the readme, because people don't read the wiki.

So stable are the "released" and tested versions (stable7, stable8 and in this case stable8.1 because of core requrements).

Correct, both here and in other core apps

And for development? How is this to be named?

I use master for core-current and dev for core-next.

In the new repository, it will be master for the next release, nothing else.

You'll get some bug fixes in the official stable branches and features and bug fixes in my fork (,purely so that the official stable branches stay super stable)

oparoz commented 9 years ago

Otherwise you do not know which PR new issues you encounter were introduced by.

Agreed, but it's just for users who want the most advanced branch. PRs can take a long time to get merged. To give you an example, I was happy to be able to use the new switch UI along with other PRs I was testing.

deMattin commented 9 years ago

@jancborchardt sorry, maybe dumb question. But core master is current on 8.2!? So I'm forced to use a testing core environment to test gallery+? As @oparoz said, I would expect an app master depending on current core and not on a developing master of core. Now I'm confused ...

oparoz commented 9 years ago

@deMattin You now understand why I've been fighting this change ;)

jancborchardt commented 9 years ago

Theoretically you could also use Gallery master with core stable8. At least that works with Mail app master – you can also use that down to stable7.

oparoz commented 9 years ago

Yes. I just decided to avoid that model of writing tons of exceptions for each version. It makes it harder for people to contribute as they would need to know about previous versions, etc. There is also the problem of missing APIs, etc. I wish to no-one else to have to reverse engineer an app to guess what everything is doing.

oparoz commented 9 years ago

So, 2) is sorted. People will be able to use my fork for core-current. It's just a bonus and will have no impact on the official app.

Regarding 1), there are a few caveats with pushing all the changes on top of the current Pictures project. None of the issues reported here will be ported over and of course there will be no commit history.

Please @DeepDiver1975 , confirm that's what you really want I'll see if I can make this repo a fork of owncloud/gallery and create a PR.

The alternative is to create the old branches needed for the build system here and keep the old repo as a commit reference.

jancborchardt commented 9 years ago

@oparoz of course, you can keep the commits. Can just be done by using a clean branch as base (which of course will then be master).

And the issues can be ported over via various tools. That is the commwnt authors will be all you, however the original authors will always be @-mentioned so everyone should be in the loop.

jancborchardt commented 9 years ago

People will be able to use my fork for core-current. It's just a bonus and will have no impact on the official app.

@oparoz what do you mean by that? Is it like with other apps now that you can use master with core master, stable81 with core stable8.1, stable8 with core stable8 etc?

oparoz commented 9 years ago

@jancborchardt OK. Will push release 13 there once it's packaged.

And regarding my fork. It's so that people can get the latest Gallery release for their production install, not just their experimental setup.

Per example. With the new branch model aligned with core, release 13 would be for 8.2 only, but using my fork, people would be able to install it on 8.0 and 8.1 if they wanted to. At some point, once I switch my production instances to 8.1, things will probably only be back ported to 8.1, etc.

oparoz commented 9 years ago

Pushed new galleryplus branch to owncloud/gallery. It's impossible to create a PR, so that branch will have to be renamed master.

oparoz commented 9 years ago

Github didn't let me, but the command line works: https://github.com/owncloud/gallery/pull/200

oparoz commented 9 years ago

Impact

oparoz commented 9 years ago

I think we've covered everything. The action plan is now here: https://github.com/owncloud/galleryplus/issues/229

Fork will be updated once the repository has been renamed.