porch: Added renderstatus to packagerevisionresources update api #3522

droot · 2022-08-31T00:49:51Z

This PR exposes kpt fn render results in the porch API (packagerevisionresources).

Note: Since API no longer returns failure on render failures, the existing tests are failing, I am looking into it but the code is good for review. The change is split in two commits to make it easier to review.

/cc @mortent @justinsb @ChristopherFry

Some follow ups:

rpkg push should display renderstatus
functionruntimes (builtin and podevaluator) should support structuredresults and fix exitCode.

droot · 2022-08-31T17:52:27Z

/cc @ChristopherFry

justinsb

Looking good ... I vote to merge and iterate :-)

justinsb · 2022-10-19T21:37:42Z

.gitignore

@@ -20,6 +20,7 @@ kpt/kpt
 *.exe
 *.gif
 bin/
+porch/cmd/porch/__debug_bin


I don't love this (I think I'd prefer a wildcard), but I guess it saves us from committing it by mistake :-)

Found the wildcard ** matches any subdirectories.

justinsb · 2022-10-19T21:46:33Z

porch/api/porch/v1alpha1/types_packagerevisionresources.go

-	Spec PackageRevisionResourcesSpec `json:"spec,omitempty"`
-	// Status PackageRevisionResourcesStatuc `json:"status,omitempty"`
+	Spec   PackageRevisionResourcesSpec   `json:"spec,omitempty"`
+	Status PackageRevisionResourcesStatus `json:"status,omitempty"`


I'm not sure which which kind we should expose this on. I'm happy for it to start here, though my guess is that PackageRevision is the more likely long-term home.

Yes. PackageRevision is the ultimate home.

justinsb · 2022-10-19T22:07:11Z

porch/api/porch/v1alpha1/types_packagerevisions.go

+// on a package resources.
+type RenderStatus struct {
+	Result fnresult.ResultList `json:"result,omitempty"`
+	Err    string              `json:"err"`


Nit: json:"error" I suggest

justinsb · 2022-10-19T22:25:11Z

porch/pkg/engine/engine.go

-		applied, task, err := m.Apply(ctx, baseResources)
+		updatedResources, taskResult, task, err := m.Apply(ctx, baseResources)
+		if taskResult != nil && taskResult.Type == api.TaskTypeEval {
+			renderStatus = taskResult.RenderStatus


Does this mean we only capture the last eval (i.e. the last function?)

But is the goal that it should contain the output/errors from every function in the pipeline? Or do we abort the run when a function return an error?

We abort the run when function returns error and if the last applied mutation was render, then we also return the renderStatus.
We can definitely extend it for all the mutations over time.

justinsb · 2022-10-19T22:30:13Z

porch/pkg/engine/init.go

 	}

-	return result, m.task, nil
+	return result, nil, m.task, nil


Nit: four retvals is likely where I start to think about a struct

along these lines, perhaps there could be a struct that contains both Task and its TaskResult?

Now returning TaskResult that contains *Task. Thanks.

natasha41575

lgtm, small question above

mortent

I agree with the merge and iterate approach, but we should be careful with the API. Probably easier to make changes in the types from the Aggregated APIserver than CRDs though.

Could we add a test of this to make sure we don't accidentally break it?

mortent · 2022-10-20T19:38:43Z

porch/pkg/engine/engine.go

+		// and is returned in packageresourceresources API's status field. We continue with
+		// saving the non-rendered resources to avoid losing user's changes.
+		// and supress this err.
+		err = nil


Do we need this block? If we don't use the err variable for anything, can't we just replace it with _ in the call to applyResourceMutations?

Good suggestion. Done. Moved the comment above it.

mortent · 2022-10-20T19:44:28Z

porch/api/porch/v1alpha1/types_packagerevisionresources.go

+// PackageRevisionResourcesStatus represents state of the rendered package resources.
+type PackageRevisionResourcesStatus struct {
+	// RenderStatus contains the result of rendering the package resources.
+	RenderStatus RenderStatus `json:"renderStatus,omitempty"`


So this only contains the result after a push right? We don't actually store the information anywhere, so a subsequent get will not contain the error? I would expect that he results from the latest run would be returned on every get. Similar to the previous comment, I would expect this to be on the PackageRevision rather than the PackageRevisionResources type.

subsequent get will not contain the error. I started with this API because this is the API that get's called when user edits a package and that's the place renderStatus is most useful. And I agree that renderStatus should be added to packagerevision's status and should also be persisted so that we get it on every get. And that can be done as a follow up incrementally.

mortent · 2022-10-20T19:46:04Z

porch/pkg/engine/engine.go

-		applied, task, err := m.Apply(ctx, baseResources)
+		updatedResources, taskResult, task, err := m.Apply(ctx, baseResources)
+		if taskResult != nil && taskResult.Type == api.TaskTypeEval {
+			renderStatus = taskResult.RenderStatus


But is the goal that it should contain the output/errors from every function in the pipeline? Or do we abort the run when a function return an error?

droot

Thanks for the feedback everyone. I have addressed all the feedback that I was planning to address in current cycle. There is definitely more follow ups but I think this is a reasonable checkpoint to get merged.

droot · 2022-10-24T20:42:29Z

Could we add a test of this to make sure we don't accidentally break it?

I enhanced the existing updateResources tests to assert the renderStatus.

droot force-pushed the porch-surface-render-results branch from 12677e0 to 9619676 Compare October 7, 2022 18:56

droot force-pushed the porch-surface-render-results branch 5 times, most recently from 20a00cf to eb83914 Compare October 19, 2022 21:13

droot changed the title ~~WIP: surface render results in packagerevisionresources api~~ porch: Added renderstatus to packagerevisionresources update api Oct 19, 2022

droot marked this pull request as ready for review October 19, 2022 21:17

droot requested review from justinsb and mortent October 19, 2022 21:17

droot assigned justinsb Oct 19, 2022

justinsb approved these changes Oct 19, 2022

View reviewed changes

natasha41575 approved these changes Oct 20, 2022

View reviewed changes

mortent reviewed Oct 20, 2022

View reviewed changes

droot force-pushed the porch-surface-render-results branch from 21c52aa to d424b55 Compare October 21, 2022 20:02

droot added 4 commits October 21, 2022 23:34

cli: render library API returns structured result

2ee3953

porch: added renderStatus to packagerevisonresources API

2cdaf73

addressed review feedback

d58e027

resolved deepgen related problems

c802d2f

droot force-pushed the porch-surface-render-results branch from d424b55 to c802d2f Compare October 21, 2022 23:35

droot added 2 commits October 24, 2022 20:05

updated e2e test to include coverage for function results

4a1d399

addressed review comment

fa33948

droot commented Oct 24, 2022

View reviewed changes

droot merged commit aa38ca6 into kptdev:main Oct 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

porch: Added renderstatus to packagerevisionresources update api #3522

porch: Added renderstatus to packagerevisionresources update api #3522

droot commented Aug 31, 2022 •

edited

Loading

droot commented Aug 31, 2022

justinsb left a comment

justinsb Oct 19, 2022

droot Oct 20, 2022

justinsb Oct 19, 2022

droot Oct 20, 2022

justinsb Oct 19, 2022

droot Oct 20, 2022

justinsb Oct 19, 2022

droot Oct 20, 2022

mortent Oct 20, 2022

droot Oct 24, 2022

justinsb Oct 19, 2022

natasha41575 Oct 20, 2022

droot Oct 20, 2022

natasha41575 left a comment •

edited

Loading

mortent left a comment

mortent Oct 20, 2022

droot Oct 24, 2022

mortent Oct 20, 2022

droot Oct 24, 2022

mortent Oct 20, 2022

droot left a comment

droot commented Oct 24, 2022

porch: Added renderstatus to packagerevisionresources update api #3522

porch: Added renderstatus to packagerevisionresources update api #3522

Conversation

droot commented Aug 31, 2022 • edited Loading

droot commented Aug 31, 2022

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 left a comment • edited Loading

Choose a reason for hiding this comment

mortent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

droot commented Oct 24, 2022

droot commented Aug 31, 2022 •

edited

Loading

natasha41575 left a comment •

edited

Loading