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

Improves odo delete for devfiles #2842

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 48 additions & 30 deletions pkg/odo/cli/component/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,36 +101,7 @@ func (do *DeleteOptions) Run() (err error) {
glog.V(4).Infof("args: %#v", do)

if experimental.IsExperimentalModeEnabled() && util.CheckPathExists(do.devfilePath) {
// devfile delete
if do.componentForceDeleteFlag || ui.Proceed(fmt.Sprintf("Are you sure you want to delete the devfile component?")) {
err = do.DevfileComponentDelete()
if err != nil {
log.Errorf("error occurred while deleting component, cause: %v", err)
}
} else {
log.Error("Aborting deletion of component")
}

if do.componentDeleteAllFlag {
if do.componentForceDeleteFlag || ui.Proceed(fmt.Sprintf("Are you sure you want to delete env folder?")) {
if !do.EnvSpecificInfo.EnvInfoFileExists() {
return fmt.Errorf("env folder doesn't exist for the component")
}
err = do.EnvSpecificInfo.DeleteEnvInfoFile()
if err != nil {
return err
}
err = do.EnvSpecificInfo.DeleteEnvDirIfEmpty()
if err != nil {
return err
}
log.Successf("Successfully deleted env file")
} else {
log.Error("Aborting deletion of env folder")
}
}

return nil
return do.DevFileRun()
}

if do.isCmpExists {
Expand Down Expand Up @@ -212,6 +183,53 @@ func (do *DeleteOptions) Run() (err error) {
return
}

// Run has the logic to perform the required actions as part of command for devfiles
func (do *DeleteOptions) DevFileRun() (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mik-dass Any plan for writing UTs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the CLI package. We are testing CLI by using integration tests I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really i am in 50-50 state whether to write UTs for cli packages or not. Recently i saw UTs for few devfile cli packages, so thought why not then covering UTs for regular cli packages. More importantly our UTs coverage are being run on all regular cli packages having no test file.

Ping @girishramnani @kadel

Copy link
Contributor Author

@mik-dass mik-dass Apr 9, 2020

Choose a reason for hiding this comment

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

Recently i saw UTs for few devfile cli packages, so thought why not then covering UTs for regular cli packages.

TBH I couldn't any. Please share some links. Also is the effort even worth it?

Copy link
Contributor

@amitkrout amitkrout Apr 9, 2020

Choose a reason for hiding this comment

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

This is a generic topic for cabal discussion i guess. I will create a separate issue #2867 to tack the discussion of whether to add UTs for cli packages are sensible or not.

So atm i am applying lgtm label for your current changes
/lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool thanks for creating the issue 👍

Copy link
Contributor

@amitkrout amitkrout Apr 11, 2020

Choose a reason for hiding this comment

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

TBH I couldn't any. Please share some links. Also is the effort even worth it?

Please refer #2867, there i have added the references.

// devfile delete
if do.componentForceDeleteFlag || ui.Proceed(fmt.Sprintf("Are you sure you want to delete the devfile component: %s?", do.EnvSpecificInfo.GetName())) {
err = do.DevfileComponentDelete()
if err != nil {
log.Errorf("error occurred while deleting component, cause: %v", err)
}
} else {
log.Error("Aborting deletion of component")
}

if do.componentDeleteAllFlag {
if do.componentForceDeleteFlag || ui.Proceed(fmt.Sprintf("Are you sure you want to delete env folder?")) {
if !do.EnvSpecificInfo.EnvInfoFileExists() {
return fmt.Errorf("env folder doesn't exist for the component")
}
if err = util.DeleteIndexFile(filepath.Dir(do.devfilePath)); err != nil {
return err
}

err = do.EnvSpecificInfo.DeleteEnvInfoFile()
if err != nil {
return err
}
err = do.EnvSpecificInfo.DeleteEnvDirIfEmpty()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should remove .odo folder also for odo delete --all, as --all signifies to delete all odo related data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

return err
}

cfg, err := config.NewLocalConfigInfo(do.componentContext)
if err != nil {
return err
}
if err = cfg.DeleteConfigDirIfEmpty(); err != nil {
return err
}

log.Successf("Successfully deleted env file")
} else {
log.Error("Aborting deletion of env folder")
}
}

return nil
}

// NewCmdDelete implements the delete odo command
func NewCmdDelete(name, fullName string) *cobra.Command {

Expand Down
6 changes: 3 additions & 3 deletions tests/integration/devfile/cmd_devfile_delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ var _ = Describe("odo devfile delete command tests", func() {

Context("when devfile delete command is executed with all flag", func() {

It("should delete the component created from the devfile and also the env folder", func() {
It("should delete the component created from the devfile and also the env and odo folders and the odo-index-file.json file", func() {
helper.CmdShouldPass("git", "clone", "https://github.com/che-samples/web-nodejs-sample.git", projectDirPath)
helper.Chdir(projectDirPath)

Expand All @@ -86,8 +86,8 @@ var _ = Describe("odo devfile delete command tests", func() {

oc.WaitAndCheckForExistence("deployments", namespace, 1)

files := helper.ListFilesInDir(filepath.Join(projectDirPath, ".odo"))
Expect(files).To(Not(ContainElement("env")))
files := helper.ListFilesInDir(projectDirPath)
Expect(files).To(Not(ContainElement(".odo")))
})
})
})