Skip to content

Special-case render so it doesn't appear as a task #3000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
justinsb wants to merge 1 commit into from
Closed

Special-case render so it doesn't appear as a task #3000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions porch/engine/pkg/engine/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ type cadEngine struct {
var _ CaDEngine = &cadEngine{}

type mutation interface {
// Apply applies the mutation to the specified resources.
// It returns the updated resources, along with the task as it should be represented in the PackageRevision spec.tasks.
// A mutation can return a nil task to indicate that we should not record a task in the PackageRevision spec.tasks.
Apply(ctx context.Context, resources repository.PackageResources) (repository.PackageResources, *api.Task, error)
}

Expand Down
10 changes: 2 additions & 8 deletions porch/engine/pkg/engine/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,8 @@ func (m *renderPackageMutation) Apply(ctx context.Context, resources repository.
return repository.PackageResources{}, nil, err
}

// TODO: There are internal tasks not represented in the API; Update the Apply interface to enable them.
return result, &api.Task{
Type: "eval",
Eval: &api.FunctionEvalTaskSpec{
Image: "render",
ConfigMap: map[string]string{},
},
}, nil
// This is an internal task not represented in the API, we return nil to represent this.
return result, nil, nil
}

// TODO: Implement filesystem abstraction directly rather than on top of PackageResources
Expand Down
10 changes: 8 additions & 2 deletions porch/repository/pkg/git/draft.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type gitPackageDraft struct {

var _ repository.PackageDraft = &gitPackageDraft{}

func (d *gitPackageDraft) UpdateResources(ctx context.Context, new *v1alpha1.PackageRevisionResources, change *v1alpha1.Task) error {
func (d *gitPackageDraft) UpdateResources(ctx context.Context, new *v1alpha1.PackageRevisionResources, task *v1alpha1.Task) error {
ch, err := newCommitHelper(d.parent.repo.Storer, d.parent.userInfoProvider, d.commit, d.path, plumbing.ZeroHash)
if err != nil {
return fmt.Errorf("failed to commit packgae: %w", err)
Expand All @@ -51,7 +51,13 @@ func (d *gitPackageDraft) UpdateResources(ctx context.Context, new *v1alpha1.Pac
ch.storeFile(path.Join(d.path, k), v)
}

message := fmt.Sprintf("Intermittent commit: %s", change.Type)
var message string
if task != nil {
message = fmt.Sprintf("Intermittent commit: %s", task.Type)
} else {
// TODO: Safe to assume it's always a render?
message = "Internal commit (render)"
}
Comment on lines +58 to +60
Copy link
Contributor

@martinmaly martinmaly Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not instead return more general type from the Apply function; it can either carry information to add a task, or internal message for non-task things.

btw, the non-task changes seem to break the model - what should constitute a task and what shouldn't?

Copy link
Contributor Author

@justinsb justinsb Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - I'll give that a go.

If we're always going to run render, I don't know that we should show it. (What happens if a user deletes it - do we just add it back?) I think in general we should show everything, but specifically here for a task we automatically add it makes the synchronization between client state and server state more complicated - the value I get on the server after an apply has an extra task. There's plenty of precedent for that in kubernetes (e.g. sidecar injection), but it does make the objects harder to work with (e.g. I think it breaks kubectl diff)

commitHash, packageTree, err := ch.commit(ctx, message, d.path)
if err != nil {
return fmt.Errorf("failed to commit package: %w", err)
Expand Down
30 changes: 19 additions & 11 deletions porch/repository/pkg/oci/mutate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,20 @@ func (p *ociPackageDraft) UpdateResources(ctx context.Context, new *api.PackageR
return fmt.Errorf("failed to write remote layer: %w", err)
}

taskJSON, err := json.Marshal(*task)
if err != nil {
return fmt.Errorf("failed to marshal task %T to json: %w", task, err)
history := v1.History{
Author: "kool kat",
Created: v1.Time{Time: p.created},
}

if task != nil {
taskJSON, err := json.Marshal(*task)
if err != nil {
return fmt.Errorf("failed to marshal task %T to json: %w", task, err)
}

history.CreatedBy = "kpt:" + string(taskJSON)
} else {
// TODO: Mark as internal in some way?
}

digest, err := layer.Digest()
Expand All @@ -170,15 +181,12 @@ func (p *ociPackageDraft) UpdateResources(ctx context.Context, new *api.PackageR
}

p.addendums = append(p.addendums, mutate.Addendum{
Layer: remoteLayer,
History: v1.History{
Author: "kool kat",
Created: v1.Time{Time: p.created},
CreatedBy: "kpt:" + string(taskJSON),
},
Layer: remoteLayer,
History: history,
})

p.tasks = append(p.tasks, *task)
if task != nil {
p.tasks = append(p.tasks, *task)
}

return nil
}
Expand Down