Skip to content

Conversation

@nghialv
Copy link
Member

@nghialv nghialv commented Dec 22, 2020

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1289

Does this PR introduce a user-facing change?:

NONE

if err := a.applicationStore.DeleteApplication(ctx, req.ApplicationId); err != nil {
switch err {
case datastore.ErrNotFound:
return nil, status.Error(codes.InvalidArgument, "The application is not found")
Copy link
Member

Choose a reason for hiding this comment

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

should be codes.NotFound 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Thanks.

@khanhtc1202
Copy link
Member

nits, should we validate the application is deleted or not before update it?
ref: https://github.com/pipe-cd/pipe/blob/a1e9f8405a635052f36eb2eb514520cc55ff5302/pkg/datastore/applicationstore.go#L135

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 33.73%. This pull request decreases coverage by -0.09%.

File Function Base Head Diff
pkg/app/api/grpcapi/web_api.go WebAPI.DeleteApplication -- 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.DeleteApplication -- 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.DisableApplication 0.00% 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.EnableApplication 0.00% 0.00% +0.00%

@nghialv
Copy link
Member Author

nghialv commented Dec 22, 2020

@khanhtc1202 You are right. Added the missing validation.

func (s *applicationStore) EnableApplication(ctx context.Context, id string) error {
return s.ds.Update(ctx, applicationModelKind, id, applicationFactory, func(e interface{}) error {
app := e.(*model.Application)
if app.Deleted == true {
Copy link
Member

Choose a reason for hiding this comment

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

haha, this is what you said.

Copy link
Member

Choose a reason for hiding this comment

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

lol

Copy link
Member Author

Choose a reason for hiding this comment

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

oops. This is not what I said. lol. Let me fix this.
What I said is what @khanhtc1202 mentioned at #1308 (comment)

@nakabonne
Copy link
Member

Thank you
/lgtm

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 33.73%. This pull request decreases coverage by -0.10%.

File Function Base Head Diff
pkg/app/api/grpcapi/web_api.go WebAPI.DeleteApplication -- 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.DeleteApplication -- 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.UpdateApplication 0.00% 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.EnableApplication 0.00% 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.DisableApplication 0.00% 0.00% +0.00%

@khanhtc1202
Copy link
Member

khanhtc1202 commented Dec 22, 2020

I have another issue that needs opinions from you guys.
Currently, the ListApplications return all Application which belongs to selected Projects (validated accessibly before query)
https://github.com/pipe-cd/pipe/blob/96f3c68d722c0c1877f8b9a5e8bb50cd49f8a121/pkg/app/api/grpcapi/web_api.go#L561-L566
I wonder, should we filter deleted application on the view (frontend) or we should filter it on web_api? How do you think about it 👀 @nakabonne @nghialv

@pipecd-bot pipecd-bot removed the lgtm label Dec 22, 2020
@nghialv
Copy link
Member Author

nghialv commented Dec 22, 2020

@khanhtc1202 Yes. I prefer to filter the deleted ones at the server side (db level by updating the query). But currently, firestore does not provide a way to list all of the applications where the deleted == true || deleted field does not exist (old data).

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 33.73%. This pull request decreases coverage by -0.10%.

File Function Base Head Diff
pkg/app/api/grpcapi/web_api.go WebAPI.DeleteApplication -- 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.DeleteApplication -- 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.EnableApplication 0.00% 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.DisableApplication 0.00% 0.00% +0.00%
pkg/datastore/applicationstore.go applicationStore.UpdateApplication 0.00% 0.00% +0.00%

@khanhtc1202
Copy link
Member

But currently, firestore does not provide a way to list all of the applications where the deleted == true || deleted field does not exist (old data).

I see, thanks for clarifying the situation 🙏
Let's address that issue with another PR 👍

@nghialv
Copy link
Member Author

nghialv commented Dec 22, 2020

Yes. That issue should be considered separately.
This PR is just for adding the RPC for @cakecatz.

@khanhtc1202
Copy link
Member

Thanks 😊
/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@pipecd-bot pipecd-bot merged commit 6e1a72c into master Dec 22, 2020
@pipecd-bot pipecd-bot deleted the app-deletion branch December 22, 2020 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow deleting application

5 participants