KfConfig should be private to coordinator.go

[jlewi]

trafficstars

I think KFConfig should be considered an internal private data structure only accessible to the packages in https://github.com/kubeflow/kubeflow/tree/master/bootstrap/pkg/kfapp

Code outside pkg/kfapp should not use KFConfig. Instead code outside of pkg/kfapp should use one of the versioned KFDef structures which is the public facing API.

The purpose of KFConfig is to provide an internal datastructure for the code in pkg/kfapp so that code can be written once but work with multiple versions of KFDef.

This is a common pattern when implementing an API; i.e. convert an external versioned data structure into an internal data structure that can evolve independently of the external API.

If code outside of kfapp uses KFConfig then it is no longer an internal datastructure it is a public API that other libraries could end up depending on.

[jlewi]

/cc @lluunn @yanniszark @gabrielwen FYI

[gabrielwen]

agree that we should move KfConfig to /kfapp only. I think the structure in overall should be:

/pkg:
  - /apis/...
    - /kfdef
      - /v1alpha1
      - /v1beta1
      - /v1
      ...
    - /plugins
      - /gcp
        - /v1alpha1
        - /v1beta1
        ...
      - /aws
      ...
  - /kfapp
    - /kfconfig
      - /converters
    ...

but for some similar fields, I'm not sure if we should have reference/dependencies. IMO I definitely don't want fields being shared between external API (KfDef and plugins) and internal data structure (KfConfig). As for shared fields between KfDef and plugins, I think it'll be reasonable if plugins are also versioned. Otherwise it'll be weird and hard to maintain to have a rolling changed data structure to depend on something that is snapshoted and versioned. So I guess my only question is: Do we want to have versioned plugin API as well?

[gabrielwen]

/assign @gabrielwen

[jlewi]

I think it would make sense for plugin specs to be versioned as well but should the version be kept in sync KFDef?

What should we do in this PR?

[gabrielwen]

I don't think they have to be in sync.

Probably not, let's try to roll this out incrementally... Will duplicate plugins to the new places gradually and change the deps afterwards.

[gabrielwen]

KfConfig has bee moved to /pkg/kfconfig, will close this after we removed the one in apis/apps

[Jeffwan]

@jlewi @gabrielwen

I may miss some context and I am a little bit confused now, we have plugins defined at

• https://github.com/kubeflow/kfctl/tree/master/pkg/kfconfig/awsplugin
• https://github.com/kubeflow/kfctl/tree/master/pkg/apis/apps/plugins/aws

What's the plan in the future? package pkg/kfapp/aws still reference to internal package "github.com/kubeflow/kfctl/v3/pkg/kfconfig/awsplugin". Why do we need pkg/apis/apps/plugins package?

[issue-label-bot[bot]]

Issue Label Bot is not confident enough to auto-label this issue. See dashboard for more details.

[jlewi]

@Jeffwan Its been a while since I've been deep in the kfctl code. I think per the original comment the plugins should have external and internal APIs datastructures to mirror the relationship between KFDef (external API) and KFConfig(internal API).

The idea is that we convert the external AP into the internal datastructure, do all processing, and then export using the external API.

This way the external API and internal representation don't need to be in lock step which makes development easier.