-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(stack): unable to delete invalid stack [EE-5753] #11813
fix(stack): unable to delete invalid stack [EE-5753] #11813
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. use a common function for isExist and simplify your comments.
// to the Kubernetes cluster API endpoint via the Portainer endpoint proxy. | ||
// See @https://github.com/portainer/portainer/blob/release/2.20/app/kubernetes/views/applications/applicationsController.js#L96 | ||
for _, manifest := range manifestFiles { | ||
if _, statErr := os.Stat(manifest); os.IsNotExist(statErr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a function for it filesystem.FileExist
api/exec/compose_stack.go
Outdated
if err != nil { | ||
return errors.Wrap(err, "failed to create env file") | ||
projectPath := stack.ProjectPath | ||
if _, err := os.Stat(projectPath); os.IsNotExist(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a function for it filesystem.FileExist
api/exec/compose_stack.go
Outdated
// copyConfigEnvVarsIfExists write the environment variables from stack configuration to target file | ||
func copyConfigEnvVarsIfExists(w io.Writer, envs []portainer.Pair) { | ||
for _, v := range envs { | ||
io.WriteString(w, fmt.Sprintf("%s=%s\n", v.Name, v.Value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should handle the error of this operation.
Nitpick: maybe use fmt.Fprintf()
directly?
api/exec/compose_stack.go
Outdated
} | ||
|
||
return "stack.env", nil | ||
} | ||
|
||
// copyDefaultEnvFile copies the default .env file if it exists to the provided writer | ||
func copyDefaultEnvFile(stack *portainer.Stack, w io.Writer) { | ||
defaultEnvFile, err := os.Open(path.Join(path.Join(stack.ProjectPath, path.Dir(stack.EntryPoint)), ".env")) | ||
func copyDefaultEnvFileIfExists(w io.Writer, defaultEnvFilePath string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func copyDefaultEnvFileIfExists(w io.Writer, defaultEnvFilePath string) error { | |
func copyDefaultEnvFile(w io.Writer, defaultEnvFilePath string) error { |
I feel like IfExists
doesn't add any value. it copied the file into the writer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is no .env file, we skip the copy and don't return error. That's the reason the IfExists
was added
api/exec/compose_stack.go
Outdated
// copyConfigEnvVarsIfExists write the environment variables from stack configuration to target file | ||
func copyConfigEnvVarsIfExists(w io.Writer, envs []portainer.Pair) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// copyConfigEnvVarsIfExists write the environment variables from stack configuration to target file | |
func copyConfigEnvVarsIfExists(w io.Writer, envs []portainer.Pair) error { | |
// copyConfigEnvVars write the environment variables from stack configuration to the writer | |
func copyConfigEnvVars(w io.Writer, envs []portainer.Pair) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
648e994
to
a877025
Compare
|
||
if err != nil { | ||
for _, manifest := range manifestFiles { | ||
if exists, err := filesystem.FileExists(manifest); err != nil || !exists { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to prevent overriding of the original error (L248), rename this variable
if exists, err := filesystem.FileExists(manifest); err != nil || !exists { | |
if exists, fileExistsErr := filesystem.FileExists(manifest); fileExistsErr != nil || !exists { |
a116d01
to
edbc333
Compare
closes EE-5753