Templates generated using Charmil CLI break due to failed factory import

[namit-chandwani]

Description

As soon as the factory was moved from core to starter, the templates generated using Charmil CLI (both by init and crud commands) started to break, since factory is placed in an internal directory of the charmil repo and hence the generated templates are not able to import it.

Logs

could not import github.com/aerogear/charmil/cli/internal/factory (invalid use of internal package github.com/aerogear/charmil/cli/internal/factory)compilerBrokenImport

Possible Solutions

• Move factory out of the internal directory

[wtrocki]

Ack.

[ankithans]

@namit-chandwani i tested it in starter and I didn't faced any issue. Just imports are incorrect. import github.com/aerogear/charmil/cli/internal/factory -> import github.com/aerogear/charmil/starter/internal/factory

[ankithans]

Looks everything works fine..

❯ go run ./cmd/cli kafka
Contains the commands to perform CRUD operations on kafka instances.

Usage:
  root kafka [command]

Examples:
cli_name kafka [create/delete/describe/list/use] [args] [flags]

Available Commands:
  create      Create a/an kafka instance
  delete      Delete a/an kafka instance
  describe    Describe a/an kafka instance
  help        Help about any command
  list        List kafka instances
  use         Use a/an kafka instance

Flags:
  -h, --help   help for kafka

Use "root kafka [command] --help" for more information about a command.

[ankithans]

and one issue @namit-chandwani can you change the name of crud locales file. It is crud.en.yaml, but it should be kafka.en.yaml or kafka.crud.en.yaml. wdyt?

[wtrocki]

@ankithans I hate to correct you but reason it works is that both are in the same repo which will not be the case when charmil is used as released artifact. Plase change starter to use releases rather than content from repo if possible

[ankithans]

I got the issue now, starter already using charmil released version. Need to update starter imports from github.com/aerogear/charmil/cli/internal/factory -> github.com/org/cliname/internal/factory

And we would need to change the template imports here -> https://github.com/aerogear/charmil/blob/main/cli/internal/template/crud/create/create.go#L6

current- "github.com/aerogear/charmil/cli/internal/factory"

should be - "github.com/org/cliname/internal/factory"

@namit-chandwani this org name and cli name can be found in go.mod of starter

[namit-chandwani]

I got the issue now, starter already using charmil released version. Need to update starter imports

@ankithans Yeah I saw you fixed this part through PR #201 now. Good work on that!  

And we would need to change the template imports here -> https://github.com/aerogear/charmil/blob/main/cli/internal/template/crud/create/create.go#L6

current- "github.com/aerogear/charmil/cli/internal/factory"

should be - "github.com/org/cliname/internal/factory"

@namit-chandwani this org name and cli name can be found in go.mod of starter

TBH even I had thought of this workaround yesterday, but the fact that it leads to a few compilation errors is what stopped me from implementing it back then.

I know you tried doing the same thing in the charmil-starter repo (ie. using placeholders in import paths) and it worked as expected. But I believe that's not working in the case of CRUD files. I'm still trying to find a valid reason behind this behavior, will put it here as soon as I find one.

---

Some more info on the issue

• Description:

  • Using placeholders in import paths in the CRUD files leads to some errors.

• Steps to reproduce the errors:

  1. Move into any of the CRUD files present in the template/crud directory. Here I'm using delete.go as an example.
  2. Add any import that makes use of placeholders. Here I'm adding a blank import as an example: import _ "github.com/aerogear/{{.Singular}}/trial"
  3. Now you'll notice an error in the run.go file which is present in the same package.

• Demo:

https://user-images.githubusercontent.com/51479159/128058756-2354adbc-5ab4-4a5c-8c6f-e8cd21c83f72.mp4

---

PS: I believe that this is not the biggest of issues that we've faced and just moving factory out of the internal directory (and using direct import paths in CRUD files) will be the simplest way to fix this. WDYT?

[wtrocki]

@namit-chandwani Lets do it. @ankithans could comment or dissagree with us on PR or after we merge :)

Edit:

I meant move from internal

[ankithans]

Using placeholders in import paths in the CRUD files leads to some errors.

definitely it will give errors, we can keep it string byte, non compilable. I think we cannot keep it as code as factory import needs to changed according to the project.

I meant move from internal

if we move factory from internal to pkg, still templates are importing/using charmil CLI's factory. We have removed factory from the core :) so it should be using it;s own project's factory right?

@namit-chandwani can you create a quick PR for this, so that I can understand the issue 😅. Because I may holding something wrong.

[namit-chandwani]

if we move factory from internal to pkg, still templates are importing/using charmil CLI's factory. We have removed factory from the core :) so it should be using it;s own project's factory right?

@ankithans Agreed! I got your point now. The method I proposed (i.e. moving factory out of internal) would no longer work as the factory was moved from core to starter because we wanted plugins to have their own factory instance, and using direct imports from the charmil repo would be the same as importing it from core, which would be defeating the entire purpose of moving it to starter.

we can keep it string byte, non compilable.

Yes, I had initially kept it this way (as byte arrays), but then changed it to actual compilable code after some discussion on my PR, since using byte arrays as templates would make it really tough to maintain.

---

@wtrocki What's your take on this?

Now that the 'moving factory out of internal' method is no longer viable, and as long as the factory is in the starter the only alternative left is to convert all the compilable template files into byte arrays. So should we consider doing this?

[wtrocki]

You type a lot :)

TL;DR mv whatever.go whatever.tmpl

Using placeholders in import paths in the CRUD files leads to some errors.

Those are not source files but templates. Why we care to compile them? https://pkg.go.dev/text/template

[namit-chandwani]

Those are not source files but templates. Why we care to compile them?

My bad, I had misunderstood this comment: https://github.com/aerogear/charmil/pull/171#discussion_r675128751

TL;DR mv whatever.go whatever.tmpl

Makes sense. Will do this, thanks!