klippy: a controller that auto-instantiates packages #3575
Conversation
justinsb
requested review from
droot,
mortent,
natasha41575 and
yuwenma
as code owners
justinsb
force-pushed
the
klippy
branch
2 times, most recently
from
c64e067
to
5bf1561
Compare
justinsb
changed the title
justinsb
force-pushed
the
klippy
branch
2 times, most recently
from
57cf6d1
to
bf15db3
Compare
| parentObjects, err := parseObjects(ctx, parentResources) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
parseObjects and parseBlueprints seems like functions could be useful in other places (porchclient++).
There was a problem hiding this comment.
Yes - I feel like we should have them somewhere already. I just don't know where...
|
|
||
| var allProposals []*api.PackageRevision | ||
| for _, blueprint := range blueprints { | ||
| proposals, err := r.buildProposals(ctx, parent, parentObjects, blueprint) |
There was a problem hiding this comment.
seems like this is the part we would like to externalize (delegate to proposal makers).
There was a problem hiding this comment.
Agreed - as we build more of these, we can refactor to share code. I would hope that anyone building the second proposal controller would refactor to reuse some of this code - hopefully making it a bit better along the way :-) cc @yuwenma
| } | ||
|
|
||
| proposals = append(proposals, proposal) | ||
| } |
There was a problem hiding this comment.
lots of binding stuff that I skipped over. I am curious, where is bind function used that was taken as input ?
There was a problem hiding this comment.
Ooops - I forgot to do that - fixed in the latest update!
|
This is quite similar to what I am looking for with #3448. The idea is that a local-config resource will describe the "my package requires", and another one in other packages will describe the "my package provides". I want a dependency resolution controller that sees these in Draft package revisions and resolves them. Dependencies themselves are also surfaced as conditions, so that we can identify when all dependencies have been satisfied. |
That's great news, thank @johnbelamaric . I do expect we'll have multiple controllers making proposals. I think it would be great if @yuwenma (or @natasha41575 ) tackled updating-the-container-image next. I don't know whether your needs are close enough that we should extend this controller, or whether we should build a second controller - generally I think we'll evolve our understanding as we build out this functionality (better to have many specialized controllers, or fewer more generic ones?) |
justinsb
force-pushed
the
klippy
branch
5 times, most recently
from
02932f8
to
f8b5ae1
Compare
justinsb
changed the title
justinsb
force-pushed
the
klippy
branch
2 times, most recently
from
ca3ded7
to
9287d91
Compare
There was a problem hiding this comment.
Looks good to me, but can we add a README file that describes what it does and includes a simple example for testing it? I've found that pretty useful in the other controllers when I need to make changes. So something like this: https://github.com/GoogleContainerTools/kpt/tree/main/porch/controllers/workloadidentitybindings/config/samples
|
|
||
| package klippy | ||
|
|
||
| //+kubebuilder:rbac:groups=porch.kpt.dev,resources=packagerevisionresources,verbs=get;list;watch;create;update;patch;delete |
There was a problem hiding this comment.
Any particular reason for putting the rbac directives here instead of in the controller.go file itself?
There was a problem hiding this comment.
Was mostly just trying it out to see how I/we felt about it. The RBAC generated is the same either way. I'll move it into the main controller, to follow the existing patterns.
| packageRevision := &packageRevisions.Items[i] | ||
|
|
||
| if !strings.HasPrefix(packageRevision.Spec.PackageName, "infra/") { | ||
| log.Info("HACK: ignoring non-infra package", "package", packageRevision.Spec.PackageName) |
There was a problem hiding this comment.
Can we document the convention used here?
There was a problem hiding this comment.
Good call, I removed the hack. Instead we only apply this where packages actually have bindings, which achieves much the same thing (scoping just to the packages that are using bindings).
This controller will automatically create draft package instances where the bindings are satisfied. If the bindings are sufficiently constraining, this can act as a guided workflow to help users do the next right thing.
|
Thanks @mortent - I added a README and was able to remove the hack. |
This controller will automatically create draft package instances where the bindings are satisfied. If the bindings are sufficiently constraining, this can act as a guided workflow to help users do the next right thing.