Skip to content
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

meteor 1.8 support #122

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

sebastianlutter
Copy link

Hi,

the current version does not support meteor 1.8. I got into trouble when I used npm bcrypt 3.0.2 with meteor-launchpad-2.3.0, binary API is incompatible with bcrypt shared object (lazy loading error).

With nodejs and mongo version updated everything is fine again. I also switched to debian stretch, and installed gosu from debian package (1.10 as the one you installed manually before)

Merge my changes if you like, and maybe release a new version 2.3.1 of your meteor launchpad so I do not need to use my own docker image :)

@jshimko
Copy link
Owner

jshimko commented Oct 27, 2018

Hey @sebastianlutter, thanks for the work here! However, I can't merge this as it is right now because you've deleted several scripts and the entire CI build directory. I realize that's probably for your own builds of your repo, but that stuff is how the automated releases happen in this repo. If you can revert those changes, I'd be happy to get your Meteor-related updates merged.

Related... once I find the free time, I'm going to do a complete rewrite of this project to be more flexible with build caching and Meteor/Node versions. For as long as this project has existed, the latest release only tends to support a few versions of Meteor before things break. I'd like to change that so it isn't such an ongoing maintenance burden for everyone.

But first, I'll settle for just getting things working with the latest version of Meteor. :)

@sebastianlutter
Copy link
Author

Hi Jeremy,
I thought a pull request just contains the commits at the time I created it. The commits of interest are the first four (cbe14fe, 65fd7df , 4c6dff9, 025329d ).

I'll revert the other commits so you can easily merge it, I just needed them for my own builds.

@sebastianlutter
Copy link
Author

Now only the meteor 1.8 relevant changes are part of my master. I added my email into Dockerfiles MAINTAINER, keep or remove it as you like. Looking forward for a v2.3.1 release :)

Once I find free time . . . I know this very well ;) If you start your rewrite feel free to drop me a line, depending on my workload I'm willing to help

best
Sebastian

@gustawdaniel
Copy link
Contributor

Now I am using meteor 1.9 with this package so this pull request seems to outdated. I propose merge or close it.

Copy link
Contributor

@gustawdaniel gustawdaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request can be closed. Launchpad works with meteor in 1.8, 1.9 and latest 2.* too.

@@ -47,7 +47,7 @@ ONBUILD ARG TOOL_NODE_FLAGS
ONBUILD ENV TOOL_NODE_FLAGS $TOOL_NODE_FLAGS

# optionally custom apt dependencies at app build time
ONBUILD RUN if [ "$APT_GET_INSTALL" ]; then apt-get update && apt-get install -y $APT_GET_INSTALL; fi
ONBUILD RUN if [ "$APT_GET_INSTALL" ]; then apt-get update && apt-get install -y apt-transport-https && apt-get install -y $APT_GET_INSTALL; fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apt-transport-https can be installed by APT_GET_INSTALL, it is not required

@@ -1,14 +1,14 @@
FROM debian:jessie
MAINTAINER Jeremy Shimko <[email protected]>
FROM debian:stretch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debian:stable is best sollution

gosu nobody true

apt-get purge -y --auto-remove wget
apt-get install -y --no-install-recommends curl bzip2 bsdtar build-essential python git gosu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, but asked community:

#140

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

Successfully merging this pull request may close these issues.

4 participants