Remove global single instance of `skaffoldOpts` in skaffold error package and use runContext Config #5537

tejal29 · 2021-03-11T21:02:07Z

Review Order:

Refactoring events to use Config interface for init #5532 - event refactor to use Config instead of variables from config

When fixing #5485 , @briandealwis suggested removing the global skaffoldOpts in error package here

In this PR

Move initErrors, buildErrors and deployErrors to individual packages.
Create Problem and allow packages to register problems. The suggestions, will switch the interface to respective Config and suggest suggestions.

- type suggestionFunc func(runCtx *runcontext.RunContext) []*proto.Suggestion
+ type suggestionFunc func(cfg interface{}) []*proto.Suggestion

Change definition of Suggestions function to run on any interface and remove singleton use of runCtx or skaffoldOpt as mentioned by @briandealwis in his review feedback.
So the following methods have changed

-func ShowAIError(err error) error {
+ func ShowAIError(cfg interface{}, err error) error {

and

-func ActionableErr(phase Phase, err error) *proto.ActionableErr {
+func ActionableErr(cfg interface{}, phase Phase, err error) *proto.ActionableErr {

ShowAIError is now called in cmd.withRunner flow where runCtx is available and handleWellknownErrors is deleted.
creatNewRunner also returns a RunContext to pass to error handling.

- func createNewRunner(out io.Writer, opts config.SkaffoldOptions) 
-    (runner.Runner, []*latest.SkaffoldConfig, error) {
+func createNewRunner(out io.Writer, opts config.SkaffoldOptions) 
   (runner.Runner, *runcontext.RunContext, []*latest.SkaffoldConfig, error) {

ActionableErr was called in event packages which used global singelton skaffoldOpts. We know pass runCtx during event.InitializeState

-func InitializeState(c []latest.Pipeline, kc string, autoBuild, autoDeploy, autoSync bool) {
+func InitializeState(cfg Config)

which is stored in eventHandler

type eventHandler struct {
	eventLog []proto.LogEntry
	logLock  sync.Mutex
+	cfg       Config
	state     proto.State
	stateLock sync.Mutex
	eventChan chan firedEvent
	listeners []*listener
}

Mutliple goroutines accessing handler.cfg should not cause a data race because this value is not only written once in InitializeState.

See https://notes.shichao.io/gopl/ch9/#avoid-writing-the-variable

The calls to ActionableErr now use this to pass the event.Config.

codecov · 2021-03-11T21:19:28Z

Codecov Report

Merging #5537 (4f16b18) into master (f280aa8) will increase coverage by 0.14%.
The diff coverage is 84.15%.

@@            Coverage Diff             @@
##           master    #5537      +/-   ##
==========================================
+ Coverage   70.54%   70.69%   +0.14%     
==========================================
  Files         410      411       +1     
  Lines       15640    15863     +223     
==========================================
+ Hits        11034    11214     +180     
- Misses       3793     3823      +30     
- Partials      813      826      +13

Impacted Files	Coverage Δ
pkg/skaffold/build/builder_mux.go	`44.73% <ø> (+10.36%)`	⬆️
pkg/skaffold/errors/problem.go	`47.61% <47.61%> (ø)`
cmd/skaffold/app/cmd/cmd.go	`63.58% <66.66%> (+0.28%)`	⬆️
cmd/skaffold/app/cmd/commands.go	`80.00% <66.66%> (-1.25%)`	⬇️
pkg/skaffold/deploy/deploy_problems.go	`73.07% <73.07%> (ø)`
pkg/skaffold/docker/parse.go	`86.19% <85.18%> (+1.86%)`	⬆️
pkg/skaffold/event/event.go	`91.12% <87.50%> (+0.01%)`	⬆️
pkg/skaffold/errors/errors.go	`90.32% <90.62%> (-2.41%)`	⬇️
pkg/skaffold/build/build_problems.go	`94.82% <94.82%> (ø)`
cmd/skaffold/app/cmd/runner.go	`60.00% <100.00%> (-0.72%)`	⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f280aa8...4f16b18. Read the comment docs.

briandealwis

The command → cmd is odd?

pkg/skaffold/errors/deploy_problems.go

briandealwis · 2021-03-11T22:14:26Z

pkg/skaffold/errors/deploy_problems.go

-func suggestDeployFailedAction(runCtx runcontext.RunContext) []*proto.Suggestion {
-	if isMinikube(runCtx.KubeContext) {
+func suggestDeployFailedAction(cfg Config) []*proto.Suggestion {
+	if isMinikube(cfg.GetKubeContext()) {


I'm fine with keeping this for now, but IMHO the config should have the cluster, which we can query if it's local, or specifically minikube or kind, etc.

And then we can have the clusters provide a status-check command. The generic one is kubectl describe nodes and kubectl get events.

i think config.ClusterType() could be a good addition so we don't have to repeat the logic many times.

+1 and we can cache the result too.

briandealwis · 2021-03-11T22:20:41Z

cmd/skaffold/app/cmd/runner.go

 	if err != nil {
 		return err
 	}

 	err = action(runner, config)

-	return alwaysSucceedWhenCancelled(ctx, err)
+	return alwaysSucceedWhenCancelled(ctx, runCtx, err)


It's odd, but I don't see this change here?

cmd/skaffold/app/cmd/runner.go

tejal29 · 2021-03-11T22:51:07Z

The command → cmd is odd?

yeah maybe a search replace did. i just noticed it.

cmd/skaffold/app/cmd/cmd.go

briandealwis · 2021-03-12T14:35:29Z

pkg/skaffold/errors/deploy_problems.go

-func suggestDeployFailedAction(runCtx runcontext.RunContext) []*proto.Suggestion {
-	if isMinikube(runCtx.KubeContext) {
+func suggestDeployFailedAction(cfg Config) []*proto.Suggestion {
+	if isMinikube(cfg.GetKubeContext()) {


+1 and we can cache the result too.

pkg/skaffold/runner/runcontext/context.go

pkg/skaffold/errors/build_problems.go

pkg/skaffold/docker/client.go

pkg/skaffold/docker/parse.go

pkg/skaffold/event/config.go

tejal29 · 2021-03-15T19:05:48Z

@briandealwis hold off until i try again removing the error config bleeding into every packages.

tejal29 · 2021-04-07T04:30:20Z

@briandealwis This is ready for review again!

briandealwis

A bunch of suggestions below, but this is a great improvement.

cmd/skaffold/app/cmd/cmd.go

cmd/skaffold/app/cmd/runner.go

pkg/skaffold/errors/problem.go

pkg/skaffold/initializer/init_problems.go

pkg/skaffold/errors/errors.go

Co-authored-by: Brian de Alwis <bsd@acm.org>

tejal29 requested a review from a team as a code owner March 11, 2021 21:02

pull-request-size bot added the size/XL label Mar 11, 2021

google-cla bot added the cla: yes label Mar 11, 2021

tejal29 mentioned this pull request Mar 11, 2021

Add Cluster Internal Service Error code along with runcontext #5491

Merged

tejal29 changed the base branch from master to refactor_event_init March 11, 2021 21:29

tejal29 changed the base branch from refactor_event_init to master March 11, 2021 21:54

tejal29 force-pushed the only_runCtx branch from b3f0bb3 to b21ffb2 Compare March 11, 2021 22:01

pull-request-size bot added size/L and removed size/XL labels Mar 11, 2021

briandealwis reviewed Mar 11, 2021

View reviewed changes

tejal29 force-pushed the only_runCtx branch 2 times, most recently from 2b47821 to 7847eb2 Compare March 11, 2021 23:51

tejal29 added this to the v1.21.0 milestone Mar 12, 2021

briandealwis reviewed Mar 12, 2021

View reviewed changes

tejal29 changed the title ~~Remove global single instance of skaffoldOpts in skaffold error package and use runContext Config~~ [Paused] Remove global single instance of skaffoldOpts in skaffold error package and use runContext Config Mar 16, 2021

tejal29 modified the milestones: v1.21.0, v1.22.0 Mar 18, 2021

tejal29 changed the title ~~[Paused] Remove global single instance of skaffoldOpts in skaffold error package and use runContext Config~~ Remove global single instance of skaffoldOpts in skaffold error package and use runContext Config Apr 7, 2021

tejal29 force-pushed the only_runCtx branch 2 times, most recently from f07f367 to 9290bf3 Compare April 7, 2021 04:27

pull-request-size bot added size/XXL and removed size/L labels Apr 7, 2021

tejal29 force-pushed the only_runCtx branch from 9290bf3 to a176919 Compare April 7, 2021 04:29

tejal29 force-pushed the only_runCtx branch 2 times, most recently from ad09c9c to 2e833c5 Compare April 7, 2021 05:42

wip

990e102

tejal29 added 3 commits April 9, 2021 09:43

so far so good.

272dde2

refactor and remove global reference

f55ff92

rebase changes

cc809af

tejal29 force-pushed the only_runCtx branch from 2e833c5 to cc809af Compare April 9, 2021 17:03

briandealwis approved these changes Apr 10, 2021

View reviewed changes

tejal29 and others added 2 commits April 14, 2021 11:41

Apply suggestions from code review as is and then fix tests.

9941e33

Co-authored-by: Brian de Alwis <bsd@acm.org>

fix tests

4f16b18

tejal29 merged commit 788275e into GoogleContainerTools:master Apr 15, 2021

tejal29 mentioned this pull request Apr 15, 2021

Revisit alwaysSucceedWhenCancelled to succeed if error is context cancelled at all times. #5686

Open

tejal29 deleted the only_runCtx branch April 15, 2021 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove global single instance of `skaffoldOpts` in skaffold error package and use runContext Config #5537

Remove global single instance of `skaffoldOpts` in skaffold error package and use runContext Config #5537

tejal29 commented Mar 11, 2021 •

edited

Loading

codecov bot commented Mar 11, 2021 •

edited

Loading

briandealwis left a comment

briandealwis Mar 11, 2021

tejal29 Mar 11, 2021

briandealwis Mar 12, 2021

briandealwis Mar 11, 2021

tejal29 commented Mar 11, 2021

briandealwis Mar 12, 2021

tejal29 commented Mar 15, 2021 •

edited

Loading

tejal29 commented Apr 7, 2021

briandealwis left a comment

Remove global single instance of skaffoldOpts in skaffold error package and use runContext Config #5537

Remove global single instance of skaffoldOpts in skaffold error package and use runContext Config #5537

Conversation

tejal29 commented Mar 11, 2021 • edited Loading

codecov bot commented Mar 11, 2021 • edited Loading

Codecov Report

briandealwis left a comment

Choose a reason for hiding this comment

briandealwis Mar 11, 2021

Choose a reason for hiding this comment

tejal29 Mar 11, 2021

Choose a reason for hiding this comment

briandealwis Mar 12, 2021

Choose a reason for hiding this comment

briandealwis Mar 11, 2021

Choose a reason for hiding this comment

tejal29 commented Mar 11, 2021

briandealwis Mar 12, 2021

Choose a reason for hiding this comment

tejal29 commented Mar 15, 2021 • edited Loading

tejal29 commented Apr 7, 2021

briandealwis left a comment

Choose a reason for hiding this comment

Remove global single instance of `skaffoldOpts` in skaffold error package and use runContext Config #5537

Remove global single instance of `skaffoldOpts` in skaffold error package and use runContext Config #5537

tejal29 commented Mar 11, 2021 •

edited

Loading

codecov bot commented Mar 11, 2021 •

edited

Loading

tejal29 commented Mar 15, 2021 •

edited

Loading