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.

Sign up for GitHub

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

Jump to bottom

odo delete component deletes components running on podman #6418

Merged
merged 13 commits into from
Dec 22, 2022
Merged

odo delete component deletes components running on podman #6418

merged 13 commits into from
Dec 22, 2022

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Dec 15, 2022 •
edited
Loading

What type of PR is this:

/kind feature

What does this PR do / why we need it:

odo delete component support for podman when devfile is present

(support for --name will be done in a separate PR)

Which issue(s) this PR fixes:

Fixes partially #6296

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Dec 15, 2022
@netlify
Copy link

netlify bot commented Dec 15, 2022 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit 224d12f
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/63a44aa05c27b60009754458

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Dec 15, 2022
@odo-robot
Copy link

odo-robot bot commented Dec 15, 2022 •
edited
Loading

Windows Tests (OCP) on commit 376cb58 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Dec 15, 2022 •
edited
Loading

OpenShift Tests on commit 376cb58 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Dec 15, 2022 •
edited
Loading

NoCluster Tests on commit 376cb58 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Dec 15, 2022 •
edited
Loading

Unit Tests on commit 376cb58 finished successfully.
View logs: TXT HTML

@feloy feloy force-pushed the feature-6296/odo-delete-component-podman branch from f440dee to d861bdb Compare December 15, 2022 14:28
@odo-robot
Copy link

odo-robot bot commented Dec 15, 2022 •
edited
Loading

Validate Tests on commit 376cb58 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Dec 15, 2022 •
edited
Loading

Kubernetes Tests on commit 376cb58 finished successfully.
View logs: TXT HTML

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Dec 16, 2022
@feloy feloy force-pushed the feature-6296/odo-delete-component-podman branch from 7a4d45a to b5ff0b5 Compare December 16, 2022 13:10
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Dec 16, 2022
@feloy feloy force-pushed the feature-6296/odo-delete-component-podman branch 2 times, most recently from 488bf32 to 3d62435 Compare December 19, 2022 09:42
@feloy
Copy link
Contributor Author

feloy commented Dec 19, 2022

Blocked by #6416
This PR needs more abstraction for component to add integration tests

@feloy feloy requested review from rm3l and removed request for anandrkskd December 19, 2022 14:10
@feloy feloy added the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Dec 19, 2022
@feloy feloy force-pushed the feature-6296/odo-delete-component-podman branch from 3d62435 to 1a1f455 Compare December 20, 2022 10:14
@feloy feloy removed the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Dec 20, 2022
@feloy feloy changed the title [wip] odo delete component deletes components running on podman odo delete component deletes components running on podman Dec 20, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Dec 20, 2022
@feloy feloy force-pushed the feature-6296/odo-delete-component-podman branch from d3266e4 to b5112ac Compare December 21, 2022 09:14
@valaparthvi valaparthvi self-assigned this Dec 21, 2022
valaparthvi
valaparthvi suggested changes Dec 22, 2022
View reviewed changes
pkg/component/delete/delete.go
Comment on lines +119 to +120
// Kubernetes cluster access fails, return with a warning only
err = clierrors.NewWarning(fmt.Sprintf("failed to get deployment %q", deploymentName), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to return a warning only if access to cluster fails, I think we should explicitly check for that error instead of returning a warning for all the errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I would also like to, but I'm not sure we can find an exhaustive and definitive lit of such error(s).

  2. As the function where it happens (ListClusterResourcesToDeleteFromDevfile) is a Devfile-related function, I would consider cluster information optional on the result, and never return an error when an error happens on the cluster, whatever the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like to, but I'm not sure we can find an exhaustive and definitive lit of such error(s).

What error is returned rn?

As the function where it happens (ListClusterResourcesToDeleteFromDevfile) is a Devfile-related function, I would consider cluster information optional on the result, and never return an error when an error happens on the cluster, whatever the error.

Yeah, I guess you're right about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also like to, but I'm not sure we can find an exhaustive and definitive lit of such error(s).

What error is returned rn?

for the case I can think of, it is a "Forbidden" error

pkg/component/delete/delete.go Show resolved Hide resolved
pkg/component/delete/delete_test.go Outdated
Comment on lines 120 to 121
podmanClient := podman.NewMockClient(ctrl)
do := NewDeleteComponentClient(kubeClient, podmanClient, execClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
Perhaps you can simply pass nil since we are not going to use the podman client here.

pkg/component/delete/delete_test.go Outdated
Comment on lines 237 to 239
podmanClient := podman.NewMockClient(ctrl)
execClient := exec.NewExecClient(kubeClient)
do := NewDeleteComponentClient(kubeClient, execClient)
do := NewDeleteComponentClient(kubeClient, podmanClient, execClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
Perhaps you can simply pass nil since we are not going to use the podman client here.

pkg/component/delete/delete_test.go Outdated
Comment on lines 711 to 713
podmanClient := podman.NewMockClient(ctrl)
execClient := exec.NewExecClient(kubeClient)
do := NewDeleteComponentClient(kubeClient, execClient)
do := NewDeleteComponentClient(kubeClient, podmanClient, execClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
Perhaps you can simply pass nil since we are not going to use the podman client here.

pkg/odo/cli/delete/component/component.go Outdated
}

if !(hasClusterResources || hasPodmanResources) {
log.Infof("No resource found for component %q in namespace %q nor in podman\n", componentName, namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.Infof("No resource found for component %q in namespace %q nor in podman\n", componentName, namespace)
log.Infof("No resource found for component %q in namespace %q or in podman\n", componentName, namespace)

Copy link
Contributor

Choose a reason for hiding this comment

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

might be nit:
I think it should only print "or in podman" if experimental mode is enabled. I would use messageWithPlatforms function here.

pkg/odo/cli/delete/component/component.go Outdated
Comment on lines 104 to 113
if !feature.IsEnabled(ctx, feature.GenericRunOnFlag) {
o.clientset.PodmanClient = nil
}
switch fcontext.GetRunOn(ctx, "") {
case commonflags.RunOnCluster:
o.clientset.PodmanClient = nil
case commonflags.RunOnPodman:
o.clientset.KubernetesClient = nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this in Complete method instead of here. WDYT?

pkg/odo/cli/delete/component/component.go Show resolved Hide resolved
pkg/odo/cli/delete/component/component.go Outdated
if err != nil {
return err
if o.clientset.KubernetesClient != nil {
log.Info("Searching resources to delete, please wait...")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move this message out of the if loop, it seems more generic.

pkg/podman/podman.go Show resolved Hide resolved
valaparthvi
valaparthvi suggested changes Dec 22, 2022
View reviewed changes
pkg/odo/cli/delete/component/component.go Outdated
Comment on lines 94 to 104
// Limit access to platforms if necessary
if !feature.IsEnabled(ctx, feature.GenericRunOnFlag) {
o.clientset.PodmanClient = nil
}
switch fcontext.GetRunOn(ctx, "") {
case commonflags.RunOnCluster:
o.clientset.PodmanClient = nil
case commonflags.RunOnPodman:
o.clientset.KubernetesClient = nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should go on L79, because if o.name == "" is satisfied, it will return early.

@sonarcloud
Copy link

sonarcloud bot commented Dec 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@valaparthvi
Copy link
Contributor

/lgtm

Thank you for working on this!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Dec 22, 2022
@openshift-merge-robot openshift-merge-robot merged commit 4617cfd into redhat-developer:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rm3l rm3l Awaiting requested review from rm3l

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@valaparthvi valaparthvi

Labels
kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@feloy @valaparthvi @openshift-merge-robot