Skip to content

Move packaging to containers and add windows-msi and linux-appimage #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Sep 26, 2019

Moves all packaging to Docker containers and adds full support for Windows MSI and Linux AppImage installers.

@pchampio
Copy link
Member

pchampio commented Sep 26, 2019

Please separate modifications into separate PRs:

  • Docker needed for containers
  • The move of functions into internal packages
  • The addition of packaging Windows MSI

I'm myself not the best at choosing what's must be put into PRs.
A good rule of thumb is on PR -> one modification idea (either it is a feature a fix...).

@provokateurin
Copy link
Member Author

I understand what you mean. But would only remove the MSI packaging part and leave the others in this PR

@provokateurin provokateurin changed the title Move packaging to containers and add windows-msi Move packaging to containers and add windows-msi and linux-appimage Sep 26, 2019
@provokateurin provokateurin force-pushed the feature/cross-packaging branch 2 times, most recently from eba3ace to b0d57fc Compare September 26, 2019 16:56
@provokateurin provokateurin force-pushed the feature/cross-packaging branch from 2c7b8bd to 3aa03cd Compare September 26, 2019 16:59
@pchampio
Copy link
Member

pchampio commented Sep 26, 2019

@jld3103 please wait form #33, I don't want to rebase twice.

hover started as a small project, more and more feature got added in top of the others, and I may confess, the code quality doesn't match the one on go-flutter.

I like the fact that you started moving function into separate files, but there is a lot of things that needs to be discussed first, I don't necessary like the overuse of internal package.
And after a quick look, I spotted some regression.

I've done some light clean-up in #22.
It adds a convenient fileutils.CopyTemplate that replaces the for loop used to write in files.

regarding moving packaging to containers, same as above, it needs discussion.

What doesn't needs discussion is windows-msi and linux-appimage.
It will be much easier if you opened a PR containing:

  1. package.go split into multiples files.
  2. linux-appimage
  3. windows-msi

@provokateurin
Copy link
Member Author

Yeah agree. Just have to look how I can separate the code changes. And yes, I think the code quality is not that great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants