-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(recipe): delete all resources belonging to a gvk from a particular namespace #101
base: master
Are you sure you want to change the base?
Conversation
pkg/job/delete_all.go
Outdated
@@ -0,0 +1,49 @@ | |||
package job |
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.
@amitbhatt818 One of the first things that I would do is to explain what this PR is all about in the PR body.
It should explain everything to a reviewer to let this reviewer help as much as possible.
Note that a reviewer may be a maintainer or a new contributor to this project. The levels of knowledge & expertise will vary. We should put all effort to explain things in a manner it can be understood by all these personas.
Can you rebase from master?
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.
Added description in the PR body. And rebased the branch.
pkg/job/delete_all.go
Outdated
|
||
func (r *TaskRunner) deleteAll() (*types.TaskStatus, error) { | ||
var message = fmt.Sprintf( | ||
"Delete: Resource %s %s: GVK %s", |
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.
The message should start as DeleteAll: ...
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.
Done
pkg/job/delete_all.go
Outdated
"openebs.io/metac/dynamic/clientset" | ||
) | ||
|
||
func (r *TaskRunner) deleteAll() (*types.TaskStatus, 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.
Do you think this should have a dedicated struct. For example:
type Deleting struct {}
func NewDeleter(config DeletingConfig) *Deleting {}
func (d *Deleting) DeleteAll() {}
func (d *Deleting) Delete() {}
pkg/job/delete_all.go
Outdated
err = client. | ||
Namespace(r.Task.DeleteAll.State.GetNamespace()). | ||
Delete( | ||
r.Task.DeleteAll.State.GetKind(), |
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.
Delete does not take kind as an argument. It takes Name.
In this case we should use DeleteAll or DeleteCollection method.
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.
Thanks 👍
pkg/job/task.go
Outdated
@@ -256,6 +265,7 @@ func (r *TaskRunner) Run() (types.TaskStatus, error) { | |||
r.tryRunAssert, | |||
r.tryRunDelete, | |||
r.tryRunApply, | |||
r.tryRunDeleteAll, |
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.
can you rearrange tryRunDeleteAll
just after tryRunDelete
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.
Resolved this comment
types/job/delete_all.go
Outdated
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
) | ||
|
||
// DeleteAll deletes the state found in the cluster |
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.
can you explain this better
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.
Changed the comment
pkg/job/delete_all.go
Outdated
"openebs.io/metac/dynamic/clientset" | ||
) | ||
|
||
func (r *TaskRunner) deleteAll() (*types.TaskStatus, 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.
Please add Unit Tests for all the changes or additions.
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.
I will add unit test for these all changes In my coming PR @AmitKumarDas
Signed-off-by: Amit Bhatt <[email protected]>
Signed-off-by: Amit Bhatt <[email protected]>
Signed-off-by: Amit Bhatt <[email protected]>
Signed-off-by: Amit Bhatt <[email protected]>
Why We Need This PR?
Some more Information:
Experiment YAML
Result
Signed-off-by: Amit Bhatt [email protected]