Package conditions #3614
Package conditions #3614
Conversation
mortent
force-pushed
the
PackageConditions
branch
3 times, most recently
from
1ad77b7 to
0310af8
Compare
mortent
requested review from
droot,
justinsb,
natasha41575 and
yuwenma
as code owners
mortent
force-pushed
the
PackageConditions
branch
2 times, most recently
from
5769fba to
1e5a45b
Compare
|
|
||
| Status ConditionStatus `yaml:"type" json:"status"` | ||
|
|
||
| Reason string `yaml:"type,omitempty" json:"reason,omitempty"` |
There was a problem hiding this comment.
Nit: yaml:"type" -> reason
We also should just remove the yaml annotations - they aren't needed by the yaml libraries that apimachinery supports - but we can do that separately!
There was a problem hiding this comment.
Fixed the incorrect names, but we can clean up the tags in a separate PR.
| draft, err := repo.UpdatePackageRevision(ctx, oldPackage) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // If any of the fields in the API that are projections from the Kptfile |
There was a problem hiding this comment.
It's fine to try this, but I suspect we won't be able to maintain this in the Kptfile going forwards.
There was a problem hiding this comment.
You mean that projecting fields from the Kptfile into the API here? If so, why? I had some concerns about it, but it actually seems to work a bit better than I feared.
| resources := map[string]string{} | ||
|
|
||
| tree, err := r.repo.TreeObject(hash) | ||
| if err == nil { |
There was a problem hiding this comment.
Nit: We should warn if the hash isn't found, and we should also check the error
There was a problem hiding this comment.
Any reason to treat hash not found different from other errors, other than perhaps trying to get a better message to the user? It seems like almost no matter what goes wrong here, an error would be a better than returning an empty map.
porch/pkg/oci/oci.go
Outdated
| key := p.Key() | ||
|
|
||
| kf, _ := p.GetKptfile(ctx) |
There was a problem hiding this comment.
Nit: we shouldn't drop errors without checking them
There was a problem hiding this comment.
Yeah, I meant to clean this up but forgot about it. Fixed now.
|
A few nits, but LGTM |
mortent
force-pushed
the
PackageConditions
branch
from
a44598f to
655e6f6
Compare
This adds support for readiness gates and conditions to kpt packages through new fields in the Kptfile, and exposes those fields in Porch through the
PackageRevisionresource.The two most important aspects about this PR is:
podReadinessGatespretty closely, so it aligns well with prior art in Kubernetes.UpstreamLockread-only through the status object, but this allows for updates too by creating patches for the changes to the Kptfile. So in the git history for the Kptfile, it shows up identical to what it looks like if someone updates the Kptfile throughPackageRevisionResources. Allowing updates through the Porch API directly simplifies updates with controllers.One aspect of the current implementation that is a bit surprising, is that kpt updates both the conditions and the readiness gates as non-associative lists. This doesn't seem correct, but unfortunately the kyaml library can't identify the merge key and therefore falls back to treating it as a non-associative list. We could handle this either by adding custom merge logic for these fields, or provide a schema that provide this information to kyaml.