feat: set up charmil core features in starter

[namit-chandwani]

Changes made

• Modified factory to include config handler and update all examples according to it (Similar to changes made in PR: refactor: include config handler in factory #153).
• Fixed some bugs
• Converted starter from a sub-module to a package (by removing go.mod and go.sum files from the starter directory)
• Set up starter CLI to use factory, i18n, logger, etc. from core instead of Rhoas SDK
• Set up config management in starter and remove the old implementation files for the same
• Added unit tests for the config package (Will keep improving them, as we proceed)
• Added some inline docs

Reasons why I converted starter from a sub-module to a package

• Replace directives will be required in a multi-module repository to allow adjacent modules to import each other's code without first publishing and remotely fetching it, which might be difficult to maintain.

• We lose the convenience of being able to run all of the unit tests in the repo for continuous integration. With a single module repository architecture, we can test all services/packages with a single go test ./… command, whereas we will require some sort of infrastructure to do the same in a multi-module repository (such as a script to run tests from each directory).

• Cobra uses a single-module repository to achieve something similar to our use case. Link: https://github.com/spf13/cobra/tree/master/cobra

• I believe that our use case does not demand the use of multiple modules, and hence a single module repo with the least amount of maintenance would be a better option for us.

References: https://github.com/golang/go/wiki/Modules#should-i-have-multiple-modules-in-a-single-repository

Possible Future Changes

Didn't make the following changes as a part of this PR, as some of these might require some discussions with the team before proceeding.

• After this PR gets merged, the only import that our starter CLI will be using from RHAOS would be that for generating command docs (github.com/redhat-developer/app-services-cli/pkg/doc). Maybe we can add a package with similar functionality to our core.

• Migrate packages like build and version to Charmil Core

• Modify the load method of config core to create an empty local config file, if it's not present already.

• Remove unwanted packages like starter/pkg/cmd/starter (debatable).

• Include the GENERATE_DOCS value inside the starter CLI config struct itself

[namit-chandwani]

Mentioning just to avoid any confusion: This PR is ready for review now :)

CC @aerogear/charmil-core

[wtrocki]

Converted starter from a sub-module to a package (by removing go.mod and go.sum files from the starter directory)

That will be problem for user who want to use starter.
In normal scenario they clone and copy folder to use it in their new repo. Now we need extra steps that are not documented or easy to figure out.

We need separate go.mod as starter should use released version of the core etc.

Reasons why I converted

Those are valid points but my take is that this is tradeoff for now
Nice that we have compiled list but starter is somewhat separate project - possible separate repo later

[wtrocki]

After this PR gets merged, the only import that our starter CLI will be using from RHAOS would be that for generating command docs (github.com/redhat-developer/app-services-cli/pkg/doc). Maybe we can add a package with similar functionality to our core.

Plugins can use upsteam cobra markdown docs.
I think we should remove this feature as it is very rhoas specific

[wtrocki]

Migrate packages like build and version to Charmil Core

Those are version and docs feature flags. We can kill it. Cobra has its own version command that we can extend (there is issue for it) but my take is to kill that with fire

Modify the load method of config core to create an empty local config file, if it's not present already.

Seems like good thing to do now

Remove unwanted packages like starter/pkg/cmd/starter (debatable).

going to be tackled in @ankithans scafollding issue/PR #67

clude the GENERATE_DOCS value inside the starter CLI config struct itself

Lets remove generate docs.completely.
Config is user facing and gen docs is for dev.

[wtrocki]

@jackdelahunt @ankithans to go with specific review and verification

[ankithans]

do we have completion package in starter or in core till now?

[namit-chandwani]

Yes, if you run the starter CLI you'll notice a command for the same over there.
BTW these docs were autogenerated by Cobra :)

[ankithans]

@namit-chandwani how can I run starter CLI?

$ go run ./starter/cmd/starter/
2021/07/14 11:06:21 open ./config.json: The system cannot find the file specified.
exit status 1

giving me this, after adding config file as well

[namit-chandwani]

@namit-chandwani how can I run starter CLI?

$ go run ./starter/cmd/starter/
2021/07/14 11:06:21 open ./config.json: The system cannot find the file specified.
exit status 1

giving me this, after adding config file as well

Move into the starter dir and run these commands:

$ make binary

$ ./starter

No need to add a config.json file as I've already added an empty one through the changes :)

[ankithans]

okay this works, but only first time it works.
After first run, it gives

$ ./starter 
2021/07/14 11:24:35 json: cannot unmarshal object into Go struct field Config.LocConfig.Files of type fs.FS

[namit-chandwani]

okay this works, but only first time it works.
After first run, it gives

$ ./starter 
2021/07/14 11:24:35 json: cannot unmarshal object into Go struct field Config.LocConfig.Files of type fs.FS

Yes, you're right.
Even I came across this bug yesterday and had added a TODO comment for the same on the code snippet which might be causing the issue.

Working on this right now

[namit-chandwani]

TODO:

• Use cobra to generate command docs instead of the RHOAS package for the same
• Remove build and version packages from starter
• Fix the error: json: cannot unmarshal object into Go struct field Config.LocConfig.Files of type fs.FS (Ref: feat: set up charmil core features in starter #158 (comment))
• Modify the load method of config core to create an empty local config file, if it's not present already
• Convert starter back to a sub-module
• Resolve build issues

[namit-chandwani]

Reasons why I converted

Those are valid points but my take is that this is tradeoff for now
Nice that we have compiled list but starter is somewhat separate project - possible separate repo later

@wtrocki If our ultimate aim is to move the starter to a separate repo, isn't now the best time to do that?

I remember that you had asked us about the same before merging that starter PR (Link: #145 (comment)), but I didn't comment as I wasn't aware of the drawbacks of a multi-module repo back then.

To do so, we can follow these steps:

1. First merge this PR, with starter as a package instead of a sub-module (ie. the way it is right now in this PR)
2. And then create a new repo and move the starter directory contents to that repo, after running these commands over there: go mod init github.com/aerogear/[starter_repo_name] and go mod tidy

I feel that it'll be better if we do this now itself, to avoid the extra amount of maintenance that will be required if we keep starter in the same repo temporarily.

WDYT?

---

Edit: Even Kubernetes maintains the starter for kubectl plugins in a separate repo: https://github.com/kubernetes/sample-cli-plugin.

CC @aerogear/charmil-core

[namit-chandwani]

@wtrocki Really sorry for following up.

Can you please have a look at the comment just above this one, whenever you're free? :)

I believe you must have missed it due to many notifications at once. TIA

[ankithans]

mainly this PR - adds test to config package and integrate it with starter + refactor some parts of starter

[namit-chandwani]

I've converted the starter dir back to a sub-module for the time being.
Perhaps we can have a more in-depth discussion regarding the starter being a sub-module vs. it being a distinct repo (as mentioned in this comment) later, so that it doesn't block our workflow now.

This PR can be merged now :)
CC @ankithans