The Wayback Machine - https://web.archive.org/web/20201101014425/https://github.com/GoogleContainerTools/skaffold/issues/4930
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

Add skaffold "InternalError" to describe an internal error. #4930

Open
tejal29 opened this issue Oct 22, 2020 · 1 comment
Open

Add skaffold "InternalError" to describe an internal error. #4930

tejal29 opened this issue Oct 22, 2020 · 1 comment

Comments

@tejal29
Copy link
Member

@tejal29 tejal29 commented Oct 22, 2020

There could be cases where due to builds happening in parallel we have now added complex structs like sync.Map.
When loading, storing updating these structs, an internal error can happen at any point.
e.g. take adding a block of code for #4896

var (
	dependencyCache = sync.Map{}
)

// dependency represents computed dependency for a dockerfile.
type dependency struct {
	files []string
	err   error
}


func GetDependencies(ctx context.Context, workspace string, dockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) {
	absDockerfilePath, err := NormalizeDockerfilePath(workspace, dockerfilePath)
	if err != nil {
		return nil, fmt.Errorf("normalizing dockerfile path: %w", err)
	}

	if v, ok := dependencyCache.Load(absDockerfilePath); !ok {
		paths, err := getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg)
		dependencyCache.Store(absDockerfilePath, dependency{
			files: paths,
			err:   err,
		})
		return paths, err
	} else if cv, ok := v.(dependency); ok {
		return cv.files, cv.err
	}
	return nil, fmt.Errorf("unexpected skaffold internal error encountered")
}

Ideally, the cached value should successfully be of type dependency. However, inorder to prevent a runtime error, the check for type conversion is necessary.
When such unexpected things happen, an error indicating skaffold internal error will be helpful.
It would be worth investigating if we should define an InternalError and see opportunities where we could use this error instead of some random error skaffold throws.

When an internal error is show to a user, skaffold should suggest user to retry with -v=trace log level and open an issue with trace log

In future, we should always generate trace logs and persist them in a file. It could be helpful in such conditions, and user won't need to re-run the same command again to grab the logs.

@MarlonGamez
Copy link
Contributor

@MarlonGamez MarlonGamez commented Oct 23, 2020

I think this would be helpful. Would give us a better idea of where to immediately look when trying to debug something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.