RELEASE.md documentation and small fixes #220
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
consideRatio
force-pushed
the
release-doc-and-small-fixes
branch
from
a230132
to
ef1ab4a
Compare
consideRatio
changed the title
consideRatio
force-pushed
the
release-doc-and-small-fixes
branch
from
ef1ab4a
to
7238f2b
Compare
|
This is fine. There's no real need for us to drop node 6 support, though nodejs itself has dropped it. If you wanted to revert that change, this is fine with me. We can save that for a major release. |
consideRatio
force-pushed
the
release-doc-and-small-fixes
branch
2 times, most recently
from
77bd229
to
1d2f24e
Compare
|
Nice okay @minrk!
|
Closes jupyterhub#218
I read in [this blog post](https://nickjanetakis.com/blog/docker-tip-59-difference-between-exposing-and-publishing-ports) about EXPOSE. Having EXPOSE in the Dockerfile doesn't automatically mean it will utilize a port on your localhost if you use `docker run` for example so it felt like a safe sane thing to do.
Before this, this test seem to be failing very weirdly as it claim there is an error but still it sais expected "asdf" but got "asdf" - something exactly identical.
consideRatio
force-pushed
the
release-doc-and-small-fixes
branch
from
797d0d7
to
d51246d
Compare
30 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
docker runbut indicates two ports in use by the software within the Dockerfile that could very well want to be published by the-Pflag or-p 8000:8000flag for example.-devHelp requested (RESOLVED)
I note that the build is failing on Node 12, yet I know that we bumped the Dockerfile to start with a
FROM node:12.13-alpinestatement recently in #213. Solving this is very relevant I figure.UPDATE: I fixed this issue in 7238f2b by relaxing the test a bit... It may be caused by an upstream bug in node or jasmine but I'm not in ready to search and fix that at the moment. The test failure said that it expected "asdf" but got "asdf" more or less, which is very weird to me. I assumed there could be some type difference etc so I started looking for if the error message contained the relevant string instead of explicitly testing for a matching error object.
Review suggestions (RESOLVED)
I stopped testing of node 6 on a gut feeling when adding node 12, should I keep support for node 6 perhaps? Should we drop testing of node 6 and also adjust package.json's constraint on the node version to be node >= 8?
RESOLVED: We continue to test and allow for node 6 to be used.