@Findus23 opened this Issue on February 20th 2017 Member

Currently I maintain all "mass icons" (browsers, os, flags, ...) in my repository https://github.com/Findus23/device-icons and create pull requests against the main piwik repository on changes.
I want to change this to directly using the repository as

  • it makes maintaining easier as someone who wants to add a icon only need to create a pull request agianst the repository
    • it doesn't bloat the main repository as binary files don't diff nicely

While submodules would be easier for testing, it seems like they are only used for optional plugins in piwik. And it would add the whole repository (not just the resized images), but this could be solved in the build script.
On the other hand, it's possible to exclude all unneeded files from github zips (see https://github.com/Findus23/device-icons/archive/1.1.0.zip), which in theory should also exclude them from composer packages. (I haven't tested it)

What would you prefer?
(PS: In both cases it might be useful to move it to the piwik organisation)

@mattab commented on February 20th 2017 Owner

Hi @Findus23

  • Using a submodule would be OK for me. (we don't only use them for plugins for example, we use submodule for Log Analytics @ https://github.com/piwik/piwik-log-analytics in piwik/misc/log-analytics)
  • Would you maybe consider migrating your device-icons repository to our Piwik organisation?
  • we'll have to exclude the files from the ZIP archives (1) using gitattributes as well as 2) updating our package build script adding a rm command to the long list)

Maybe other people have other thoughts? cc @tsteur @sgiehl

@Findus23 commented on February 20th 2017 Member

Would you maybe consider migrating your device-icons repository to our Piwik organisation?

As mentioned I don't mind but currently I am not able to as I don’t have the permission to create repositories on piwik.

we'll have to exclude the files from the ZIP archives (1) using gitattributes as well as 2) updating our package build script adding a rm command to the long list)

Are you sure that (1) is necessary as it seems like Github doesn't include submodules in ZIP archives. (just empty directories)

@sgiehl commented on February 20th 2017 Member

As mentioned I don't mind but currently I am not able to as I don’t have the permission to create repositories on piwik.

You should be able to do that nevertheless. Within your repo setting you should have possibility to "transfer ownership". That way you should be able to move it to piwik.

@Findus23 commented on February 20th 2017 Member

Oddly it seems like this isn't possible

Users must have admin or owner permissions within the receiving organization before they can transfer a repository that they individually own.
https://help.github.com/articles/transferring-a-repository-owned-by-your-personal-account/#transferring-to-an-organization

But I should be able to transfer it to you and then you can transfer it to piwik.

@sgiehl commented on February 20th 2017 Member

Got a request for transfer, but can't accept it. Getting a 500 error when visiting the confirmation page. But you might add me as collaborator within your repo settings. Maybe I'm able to transfer it on my own.

@Findus23 commented on February 20th 2017 Member

Getting a 500 error when visiting the confirmation page
That's really strange.

BTW: While we're on it, has anyone an idea for a better name? device-icons doesn't really fit anymore.

@sgiehl commented on February 20th 2017 Member

Nope. doesn't work either. Seems I can't enter the settings as collaborator.
I could create a new repo under piwik with the same name and give you push access, so you can push the repo there.

@Findus23 commented on February 20th 2017 Member

I could create a new repo under piwik with the same name and give you push access, so you can push the repo there.

That would work

@sgiehl commented on February 20th 2017 Member

Just created it. You should have admin access to this new repo https://github.com/piwik/device-icons

@Findus23 commented on February 20th 2017 Member

EDIT: Forgot to accept invite

@Findus23 commented on February 20th 2017 Member

Seems to be working

@mattab commented on February 20th 2017 Owner

Name: how about piwik-icons? :sunglasses:

@Findus23 commented on February 20th 2017 Member

how about piwik-icons? :sunglasses:

Sounds good (and descriptive)
Which creates a new question: Where would you add the submodule? /plugins/icons/? It isn't really a plugi.

@mattab commented on February 20th 2017 Owner

How about misc/icons ?

@sgiehl commented on February 20th 2017 Member

we could also add that within an plugin like plugins/Morpheus/icons.
Not sure if some people might restrict access to the misc folder, as it's not really needed. Only for a custom logo afaik.

@mattab commented on February 20th 2017 Owner

I didn't think of the fact that icons will be loaded from the Web app UI. I thought that we would still copy all icons to the right places / in the right plugins folders ?

@sgiehl commented on February 20th 2017 Member

Sue, if we copy them, it's not a problem. Not sure how it will be handled...

@Findus23 commented on February 20th 2017 Member

I'm not sure if we should copy them as this creates many directories whose source isn't obvious.
I'd prefer plugins/Morpheus/icons

@mattab commented on February 20th 2017 Owner

By copying I meant only copy those directories within dist https://github.com/piwik/device-icons/tree/master/dist into these directories where the icons currently reside (as per the dist sub-listings). ie. could be done by running a simpleshell script. The idea would be to keep a meaningful icons path where icons would still sit in the related plugins and not all in Morpheus theme.

But maybe that's not too useful and we could put all icons in the Morpheus theme.

@Findus23 commented on February 20th 2017 Member

I agree plugins/Morpheus/icons/dist/DevicesDetection/images/os/AND.png is overly complicated and redundant. Maybe copying is really simpler. Only disadvantage is that everybody, who wants to use (or develop) piwik from git, needs to run this script. So it could help if it isn't a bash script, but a cli php script.

@mattab commented on February 20th 2017 Owner

Only disadvantage is that everybody, who wants to use (or develop) piwik from git, needs to run this script. So it could help if it isn't a bash script, but a cli php script.

Good point re: PHP, like a console command in the development: scope where we already have similar commands eg. development:sync-system-test-processed / development:sync-ui-test-screenshots.

maybe not everybody would have to run this script, as it would only be run when some icons are updated or so, which would be not so frequent and likely done by just a few team members.

@Findus23 commented on February 20th 2017 Member

maybe not everybody would have to run this script, as it would only be run when some icons are updated or so, which would be not so frequent and likely done by just a few team members.

Yes, but everyone (also travis) needs to run it once after git clone, otherwise they don't see any icons.

@mattab commented on February 20th 2017 Owner

we need those icons in the git repo without having to run any command, because it's expected that a fresh Piwik git clone will just work out of the box. So we'll have to add/push them to the repo as well.

So far I see two options

  • if we don't use the submodule, we would just a console command to copy those files in Morpheus
  • if we use a submodule we can put all icons in Morpheus
@Findus23 commented on February 20th 2017 Member

If we add them to the git repository it gets much more complicated.
But we can go the opposite way:

That way a git clone (with clone of sub directories "just work") and we only need to remove the source images on build.

@mattab commented on February 21st 2017 Owner

@Findus23 Your last proposal sounds good and best so far :+1: I think it would work nicely!

@mattab commented on March 28th 2017 Owner

New piwik-icons repository and project: https://github.com/piwik/piwik-icons :tada:

@mattab commented on March 28th 2017 Owner

Done in All icons in own repo and included as a submodule #11548 and later in https://github.com/piwik/piwik/pull/11272

Closing but in case i've missed something please reopen.

This Issue was closed on March 28th 2017
Powered by GitHub Issue Mirror