Skip to content

Conversation

filipeforattini
Copy link
Contributor

Hello @matutter! Nice to meet you!

First of all: thanks! I really appreciate your effort on this repository, well done man! This small pack of yours really helped me with few daily builds with small dynamic parameter changes, you have no idea!

I'm here proposing just a small change, I hope we can have a small discussion on those items below:

  • The file.expose() method on current Dockerfile version does not accept the syntax with an array of ports. So for you to expose more than one at once it should look like: EXPOSE 80/tcp 443/tcp; separated by space. Currently yours generates EXPOSE [ 80/tcp, 443/tcp ] and I keep getting Invalid containerPort ]. More at docs!
  • Added at docs file.user() which is a huge deal for containers I use. And the best part: you have done that! Haha, why is it not there? 😄
  • Added on .travis.yml Node's versions 8, 10;

Thanks to you I can already see everything working well with the travis config you've done (https://travis-ci.org/drentrega/dockerfilejs/jobs/410836118).

Greetings from São Paulo!
Filipe

@coveralls
Copy link

coveralls commented Aug 1, 2018

Coverage Status

Coverage remained the same at 94.502% when pulling 17b13eb on drentrega:master into 7d8b71d on matutter:master.

@matutter matutter merged commit 17b13eb into matutter:master Aug 28, 2018
@matutter
Copy link
Owner

@filipeforattini, thanks for the email, I don't really check github much. With the exception of the editor artifacts (editorconfig) I've accepted this request, version 1.0.7 is published on npmjs.

Thank you.

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.

3 participants