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

Copy Dockerfile from the right spot #1623

Merged
merged 1 commit into from Jun 3, 2021
Merged

Conversation

cromedome
Copy link
Contributor

I tested this on a different machine than I developed on prior to release, and had some cruft that masked a bug when adding a Dockerfile to a new application. The dockerfile is located off of the dist dir, not off of the skel dir. This minor fix changes that.

I consider this a blocker for the release. Please review ASAP.

I tested this on a different machine than I developed on prior to
release, and had some cruft that masked a bug when adding a Dockerfile
to a new application. The dockerfile is located off of the dist dir, not
off of the skel dir. This minor fix changes that.

I consider this a blocker for the release. Please review ASAP.
Copy link
Member

@bigpresh bigpresh left a comment

Choose a reason for hiding this comment

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

Looks sane - but does beg the question of whether the Dockerfile actually ought to be under the skel/ directory? (If for no other reason, being in the skel directory would let the user point at their own skel directory containing a customised Dockerfile that does their own particular needful, no?)

@cromedome
Copy link
Contributor Author

cromedome commented Jun 2, 2021

Looks sane - but does beg the question of whether the Dockerfile actually ought to be under the skel/ directory? (If for no other reason, being in the skel directory would let the user point at their own skel directory containing a customised Dockerfile that does their own particular needful, no?)

I don't disagree. I put the Dockerfile in a separate directory because I didn't want to include it automatically when everything from skel gets copied over. Having it in a separate directory gives us a place to include other configuration for Docker should the implementation ever change, if we wanted to add a docker-compose file, different flavors of the Dockerfile, etc.

We could always take the opposite approach and put it in skel and delete it from the new application directory if --docker wasn't specified, but I don't like this approach. People with a custom skel directory could include their own Dockerfile and ignore the option entirely.

@bigpresh
Copy link
Member

bigpresh commented Jun 2, 2021

I don't disagree. I put the Dockerfile in a separate directory because I didn't want to include it automatically when everything from skel gets copied over.

Ah yes, everything from skel comes over automatically whether you're using Docker or not, doesn't it? In that case, that would not be a good place to put this, and the "it gets copied over but then deleted if we're not going to use it feels a bit... I dunno, hacky.

@cromedome
Copy link
Contributor Author

Ah yes, everything from skel comes over automatically whether you're using Docker or not, doesn't it? In that case, that would not be a good place to put this, and the "it gets copied over but then deleted if we're not going to use it feels a bit... I dunno, hacky.

That's why I didn't do it that way :)

@racke
Copy link
Member

racke commented Jun 2, 2021

It is really small, maybe we can inline it in the CLI module?

@cromedome cromedome merged commit 941edb6 into master Jun 3, 2021
@cromedome cromedome deleted the bugfix/copy-dockerfile branch June 3, 2021 13:23
cromedome added a commit that referenced this pull request Jun 3, 2021
    [ BUG FIXES ]
    * GH #1611: Redirect '/' doesn't always work as expected (Russell
      @veryrusty Jenkins, Christopher Gurnee)
    * PR #1620: Quiet spammy failing CI builds (Jason A. Crome)
    * PR #1623: Copy Dockerfile from the right spot (Jason A. Crome)

    [ ENHANCEMENTS ]
    * PR #1613: Add git features to Dancer2 CLI (Jason A. Crome)
    * PR #1614: Generate Dockerfile when creating new app (Jason A. Crome)

    [ DOCUMENTATION ]
    * PR #1563: Fix typos in perlcritic.rc notes (Achyut Kumar Panda)
    * PR #1609: Document and test for missing DSL keywords (racke, Jason A.
      Crome)
    * PR #1618: Provide a consistent list of community resources (Jason A.
      Crome)
    * PR #1619: Clarify Dancer2::Template::Simple's role in life (Jason A.
      Crome)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants