Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Replace packr and packd in gobuffalo cli with embed.FS #48

Merged
merged 20 commits into from
Nov 20, 2021
Merged

Replace packr and packd in gobuffalo cli with embed.FS #48

merged 20 commits into from
Nov 20, 2021

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Nov 16, 2021

I started migrating a bunch of files away from packr to embed already, but some are still missing. This PR is just here to keep track of the changes and allow for early feedback.

I needed to upgrade gobuffalo/genny to be compatible with io/fs and other golang 1.16 features: gobuffalo/genny#42

Next steps:

  • the gobuffalo/buffalo codebase also needs to be migrated from packr to embed.FS
  • update cli to new gobuffalo codebase:
    • update templates to fit new codebase
    • update internal coke project (used in tests) to newly generated templates
    • add fix routines to aid in migration

@fasmat
Copy link
Member Author

fasmat commented Nov 18, 2021

In 1.16 files and directories starting with a dash (e.g. -dot-env.tmpl) cannot be embedded. This seems to have been fixed in 1.17 (although files starting with _ or . still can only be embedded when listed explicitly in the glob for the //go:embed directive).

I used a custom genny.Transformer function which can be removed again when gobuffalo drops support for 1.16 in Feb 2022. I added TODOs in the code so we don't forget why it was added in the first place.

The next steps:

  • Clarify some open points in my changes (see comments I added)
  • Add a few missing tests (see comments I added)
  • Migrate gobuffalo/buffalo from packr to embed.FS
  • Update coke project to most recent code base
  • Add fix routines to help current users of gobuffalo with the migration from older versions to the current
  • Code Review

Copy link
Member

@paganotoni paganotoni left a comment

Choose a reason for hiding this comment

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

LGTM @fasmat, thanks for putting this together. Left a couple of comments on things that would be nice to have and future tasks. It was a nice excercise to review this PR.

@@ -0,0 +1,100 @@
module coke

go 1.17
Copy link
Member

Choose a reason for hiding this comment

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

Would this work when building against Go 1.16 (We still run tests there).

Copy link
Member Author

@fasmat fasmat Nov 20, 2021

Choose a reason for hiding this comment

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

Yes, this is just a newly generated project with cli and it defaults to the installed go version (1.17 in my setup).

I will update that again when gobuffalo/buffalo is completely free of packr and then tackle the Todo that mentioned generating and building a new buffalo project in a test


switch opts.Style {
case "multi":
g.Box(packr.New("github.com/gobuffalo/buffalo/multi", "../docker/templates/multi"))
multi, err := fs.Sub(templates, "templates/multi")
Copy link
Member

Choose a reason for hiding this comment

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

There is a follow up action from this one: moving to only have multi-stage docker file. That would simplify this generator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I've seen #42 but we should resolve that task in a dedicated PR. I didn't want to do too much at once.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm on that one.

@paganotoni paganotoni changed the base branch from main to development November 20, 2021 17:34
@paganotoni paganotoni merged commit b1ab4ec into gobuffalo:development Nov 20, 2021
@fasmat fasmat deleted the feature/remove-packr-and-packd branch November 20, 2021 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants