Introduce platform.PostApplyHook interface and implement it for AKS cluster

[invidian]

See commit messages for the details.

Closes #855

[knrt10]

Just a NIT, rest LGTM.

[knrt10]

+1

[iaguis]

Looks OK but I'd like to have @johananl's review before merging.

[johananl]

I'm fine with this implementation for now if it solves the problem for us. Once we refactor the platform interface I think it could make more sense to have a method called PostApplyHook() (or even PostApplyHooks()?) on the platform interface which optionally returns the hook(s) to be executed. Doing so would make things more explicit compared to implementing another interface.

[invidian]

I'm fine with this implementation for now if it solves the problem for us. Once we refactor the platform interface I think it could make more sense to have a method called PostApplyHook() (or even PostApplyHooks()?) on the platform interface which optionally returns the hook(s) to be executed. Doing so would make things more explicit compared to implementing another interface.

Since the hook is optional, I think it's nice to have it separate, as it allows us to avoid adding boilerplate code to all platforms. This is not something every platform MUST implement, like e.g. generating Terraform code or Terraform plan (this could actually be optional too).

[johananl]

I'm fine with this implementation for now if it solves the problem for us. Once we refactor the platform interface I think it could make more sense to have a method called PostApplyHook() (or even PostApplyHooks()?) on the platform interface which optionally returns the hook(s) to be executed. Doing so would make things more explicit compared to implementing another interface.

Since the hook is optional, I think it's nice to have it separate, as it allows us to avoid adding boilerplate code to all platforms. This is not something every platform MUST implement, like e.g. generating Terraform code or Terraform plan (this could actually be optional too).

The price you're paying is clarity: Having an interface which can be optionally implemented leaves things implicit and more "magical" as far as the caller is concerned. It also doesn't make sense to me conceptually at the abstraction level: "A platform is a type of hook?" 🤔 This is very different from saying "AWS is a type of platform". Go interfaces are built for certain purposes and I don't think this use case is one of them. I think a hint which supports this is that you had to type-assert the interface rather than just executing a generic method.

I have no problem returning nil or an empty slice when no hooks are necessary on all platforms. To me this is much better than bending the abstraction to avoid "duplicating" a single line per platform.

[invidian]

I have no problem returning nil or an empty slice when no hooks are necessary on all platforms. To me this is much better than bending the abstraction to avoid "duplicating" a single line per platform.

Well, that's exectly the boilerplate I'm talking about. And it's not single line, but at least 4 😄

The price you're paying is clarity: Having an interface which can be optionally implemented leaves things implicit and more "magical" as far as the caller is concerned. It also doesn't make sense to me conceptually at the abstraction level: "A platform is a type of hook?" thinking This is very different from saying "AWS is a type of platform". Go interfaces are built for certain purposes and I don't think this use case is one of them. I think a hint which supports this is that you had to type-assert the interface rather than just executing a generic method.

Yeah, I agree that abstraction wise this is not great. (Actually, initially my interface was named PlatformPostApplyHook, but then linter complains, that you do platform.PlatformPostApplyHook.

And what do you think about:

type PlatformWithPostApplyHook interface {
  Platform
  PostApplyHook(kubeconfig []byte) error
}

I think it provides much better context then, as it extends the Platform interface and optionally ships this extra method. Name is something we should discuss of course :)

[iaguis]

I think this discussion doesn't belong to this PR :)

[invidian]

I think this discussion doesn't belong to this PR :)

Well, I consider now changing this newly introduced interface to the version from #886 (comment) 😄

[johananl]

For the sake of moving this forward @invidian, please do whatever makes sense to you which solves the problem for now, and I'll do my best to integrate whatever you come up with into the new platform interface in a way which makes sense. We can continue the discussion there.