Skip to content
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.

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

Explore removing latestv1.Pipeline and latestv1.Artifact from runner & runcontexts everywhere. #5797

Closed
tejal29 opened this issue May 6, 2021 · 1 comment · Fixed by #6068
Assignees
Labels
kind/todo implementation task/epic for the skaffold team planning/Q3-21 priority/p1 High impact feature/bug.
Milestone

Comments

@tejal29
Copy link
Contributor

tejal29 commented May 6, 2021

Follow up from #5781 PRs

Support Skaffold cmd parser to have multiple pipeline runners.

We are adding a new render phase in a v2 runner.
This runner should be created when a V3 config is detected. However this is not possible right now.

The runner interface has latestV1 config embedded in it.

type Runner interface {
	Apply(context.Context, io.Writer) error
	ApplyDefaultRepo(tag string) (string, error)
	Build(context.Context, io.Writer, []*latestV1.Artifact) ([]graph.Artifact, error)
	Cleanup(context.Context, io.Writer) error
	Dev(context.Context, io.Writer, []*latestV1.Artifact) error
	Deploy(context.Context, io.Writer, []graph.Artifact) error
	DeployAndLog(context.Context, io.Writer, []graph.Artifact) error
	GeneratePipeline(context.Context, io.Writer, []*latestV1.SkaffoldConfig, []string, string) error
	HasBuilt() bool
	HasDeployed() bool
	Prune(context.Context, io.Writer) error
	Render(context.Context, io.Writer, []graph.Artifact, bool, string) error
	Test(context.Context, io.Writer, []graph.Artifact) error
}

Similarly see RunContext has latestv1.Pipeline embedded in the struct.

The current V2 runner has the following methods


Dev(ctx context.Context, out io.Writer, artifacts []*latestV1.Artifact) error
GeneratePipeline(ctx context.Context, out io.Writer, configs []*latestV1.SkaffoldConfig, configPaths []string, fileOut string) error
Render(ctx context.Context, out io.Writer, builds []graph.Artifact, offline bool, filepath string) 

where []graph.Artifact has latestV1.Artifact field.

However, the V2 runner is expected to see a latestV2.Artifact and latestV2.SkaffoldConfig. If we make that change, the v2 runner will not adhere by runner.Runner interface.
This will mean, we will have to duplicate all code in cmd package e.g. cmd.createNewRunner to return a v2 runner.

It would be great

  1. if we can keep the code in cmd package as is
  2. have less duplication in pkg/runner.Build functions as build is going to be the same.

Device a plan to remove the version embedded in Runner interface, RunContext struct.

@tejal29 tejal29 added this to the v1.25.0 milestone May 6, 2021
@tejal29 tejal29 added kind/todo implementation task/epic for the skaffold team priority/p1 High impact feature/bug. labels May 6, 2021
@tejal29 tejal29 modified the milestones: v1.25.0, v1.26.0 May 24, 2021
@tejal29 tejal29 modified the milestones: v1.26.0, v1.28.0 Jun 7, 2021
@tejal29
Copy link
Contributor Author

tejal29 commented Jun 10, 2021

@tejal29 to add more description.
Should be done by v1.28.0 milestone to make sure we can achieve the target date of july end for skaffold render.

@tejal29 tejal29 added planning/Q4-21 Q4 2021 planning and removed planning/Q2-21 labels Jun 10, 2021
@tejal29 tejal29 changed the title Explore removing latestv1.Pipeline and latestv1.Artifact from runner methods Explore removing latestv1.Pipeline and latestv1.Artifact from runner & runcontexts everywhere. Jun 25, 2021
@tejal29 tejal29 added planning/Q3-21 and removed planning/Q4-21 Q4 2021 planning labels Jun 26, 2021
@tejal29 tejal29 reopened this Jul 14, 2021
@tejal29 tejal29 modified the milestones: v1.28.0, v1.29.0 Jul 14, 2021
@tejal29 tejal29 closed this as completed Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/todo implementation task/epic for the skaffold team planning/Q3-21 priority/p1 High impact feature/bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants