Skip to content

Conversation

@Szarny
Copy link
Contributor

@Szarny Szarny commented Jun 3, 2022

What this PR does / why we need it:

According to the #3534, it's useful that there is the feature to restart piped from the web console.

To implement this feature, I have added new definitions for the RestartPiped Command and the associated gRPC and Web API. Once this is merged, we can implement the logic of the API using the RestartPiped Command later.

Note that this PR does not implement any specific API or UI changes, so there are no user-facing changes.

Which issue(s) this PR fixes:

Fixes partially #3534

Does this PR introduce a user-facing change?:

NONE

@Szarny Szarny changed the title Add feature to restart Piped via web console (gRPC) Add RestartPiped Command gRPC to Web API Jun 3, 2022
option (model.role).project_role = ADMIN;
}
rpc RestartPiped(RestartPipedRequest) returns (RestartPipedResponse) {
option (model.role).project_role = ADMIN;
Copy link
Member

Choose a reason for hiding this comment

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

I guess the EDITOR role is enough to make changes like this

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 fixed it!

Copy link
Member

@nghialv nghialv Jun 6, 2022

Choose a reason for hiding this comment

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

@khanhtc1202 @Szarny I prefer ADMIN over EDITOR.
If only looking from the permission, EDITOR is enough to do.
But from the point of responsibility and management, the Admin who created those Piped, who is managing those Piped. And Restart is just one of their management operations.

WDYT?

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 am not familiar with PipeCD Role yet, but I think it would be more natural to revert to ADMIN if the authority for Piped registration and version up is tied to ADMIN role.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I can agree with that 🤔 In my imagination, there will be only one ADMIN for each team/project and several EDITOR (the product devs). Then it would be nice if the EDITOR - who is in charge of deployment can handle restart piped or not by themself. And besides, I think this function does not that powerful or dangerous for the deployment of pipecd application, so EDTIOR is good to me. Happy to hear other thoughts 😄

Copy link
Member

Choose a reason for hiding this comment

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

there will be only one ADMIN for each team/project

Actually, many projects are having multiple admins.
Even in the case of 1 admin, I don't think that the number of admins matters here.

Then it would be nice if the EDITOR - who is in charge of deployment can handle restart piped or not by themself
this function does not that powerful or dangerous for the deployment of pipecd application

As our current design, Piped is installed and managed by ADMIN.
EDITOR users only care about the deployment. They don't even care about the Piped status. So why should we give that unneeded ability to them?

That operation is not so dangerous but at least we shouldn't give something to people who don't want it. Besides that, we can limit the miss operation from them.

So if there is no special reason to give that operation to EDITOR, we should keep our consistency as current: all Piped operations belong to ADMIN.
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/server/service/webservice/service.proto#L40-L67

These are my points. But TBH, I don't have a strong opinion. I feel fine for both EDITOR and ADMIN. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification, after seeing the current piped operations and roles required to do them, I think it would be better to make that ADMIN role required operation (RestartPiped should be as same as UpdatePipedDesiredVersion and Enable/DisablePiped) as 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.

👍

Copy link
Member

Choose a reason for hiding this comment

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

cc @Szarny

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 updated it to ADMIN 👍

khanhtc1202
khanhtc1202 previously approved these changes Jun 3, 2022
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Here you go

@khanhtc1202 khanhtc1202 changed the title Add RestartPiped Command gRPC to Web API Add RestartPiped gRPC to Web API Jun 3, 2022
@khanhtc1202
Copy link
Member

@Szarny Changed my mind about my approval. Since this PR added the RestartPiped command already, thing that left on the server-side for this function is only to implement the RestartPiped gRPC (in WebAPI) which only create RestartPiped command and store it to the datastore. So I think it would be better to wrap that implementation in this PR too. Just want to reduce unused code amsp, because it looks a bit weird to me if we define a lot of things but nothing been used yet (gRPC and model but nothing linked between them)
You can refer to the implementation of SyncApplication gRPC for the implementation of this RestartPiped gRPC
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/server/grpcapi/web_api.go#L641

@Szarny
Copy link
Contributor Author

Szarny commented Jun 6, 2022

Added Web API implementation: 7384146

Copy link
Member

@nghialv nghialv left a comment

Choose a reason for hiding this comment

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

Thank you. There you go!

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

🙌

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.

3 participants