search

alexellis / arkade

Open Source Marketplace For Developer Tools
https://blog.alexellis.io/kubernetes-marketplace-two-year-update/
MIT License
4.24k stars 287 forks source link

Remove duplication via Go chaining/structs/WithOptions() #21

Open alexellis opened 4 years ago

alexellis commented 4 years ago

Description

There are opportunities to remove duplication between each app through the use of method chaining / fluent APIs and the use of structs or WithOptions().

Chaining:

app := arkade.App{}

app.SetArch("x86_64")
.SetRepo("https://x.com/etc")
.SetNamespace("default")
.SetKubeconfig(kubeconfigPath)

err, helpMessage:= app.Install()

Structs could reduce custom variables and flags parsing:

type App struct {
  ChartRepo string
  ChartName string
}

WithOptions:

containerd has a non-fluent API that is not chained, but passed in via a series of extra args:

chart.Install(arkade.WithArch("x86_64"), arkade.WithOverride("basic-auth", "true"))

At some later point, a manifest file, or a series of manifests could populate this information from an external source, to remove hard-coded paths/variables/flags from the codebase.

Waterdrips commented 4 years ago

I think this is a great idea, we could get it into one app we know well (like inlets-operator or openfaas) and work out the kinks?

What about something to "automate" getting kubeconfig, cluster/client ARCH and such things too?

alexellis commented 4 years ago

That approach sounds ideal, incremental and experimental, then we could include more after that.

LucasRoesler commented 4 years ago

Who is responsible for validating the value that is passed in? I don't think this approach would allow the SetFoo method to validate the value and return an error because the signature would have to be 

func (a *App) SetFoo(value string) (a *App, err error) {}

but for chaining to work, it needs to be 

func (a *App) SetFoo(value string) a *App {}

I guess we could add a separate

func (a *App) Validate() error {}

or this is returned from the Install method?

aidun commented 4 years ago

I introduced the app type and some cleanups in #24 maybe you can take some parts of it.

aidun commented 4 years ago

Hi all, I provided a implementation proposol for the App type in #46 and a like to make a draft pr with a sample app for helm 3. It would be no problem, because I already did it in #24 and can recycle some parts. What do you think @Waterdrips @alexellis

aidun commented 4 years ago

@alexellis I was working on this issue before as you can see. I saw also the changes made for the Loki app, they are targeting in the same direction. The details from #129 are additions to it.

aidun commented 4 years ago

@Waterdrips I added a solution for pre and post tasks to draft pr, to show how it could work.

aidun commented 4 years ago

@Waterdrips @alexellis should I work on it more in my sparetime or is it a task with low priority?

Waterdrips commented 4 years ago

I like the implementation - it would be potentially more valuable to start porting some of the apps over to the existing new style.

Probably pick some of the simpler apps (without pre and post steps).

aidun commented 4 years ago

I saw no specific issue for that. If it is okay, I will raise issues to migrate some apps. I can do some of the PRs to, but not all.

Can we label the issues as good first issue?

Waterdrips commented 4 years ago

sure - if you mention this issue in the ones you open I can go and add the label.