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

Implement RPC to retrieve planpreview results #2121

Merged
merged 2 commits into from Jun 24, 2021
Merged

Conversation

nghialv
Copy link
Member

@nghialv nghialv commented Jun 24, 2021

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #2107
Fixes #2108

Does this PR introduce a user-facing change?:

NONE

Copy link
Collaborator

@pipecd-bot pipecd-bot left a comment

Choose a reason for hiding this comment

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

GO_LINTER

Some issues were detected while linting go source files in your changes.

pkg/app/api/commandoutputstore/store.go Outdated Show resolved Hide resolved
@pipecd-bot
Copy link
Collaborator

GO_LINTER

The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by /golinter trigger command right now.

You can check the build log from here.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.53%. This pull request decreases coverage by -0.12%.

File Function Base Head Diff
pkg/app/api/commandoutputstore/store.go NewStore -- 0.00% +0.00%
pkg/app/api/commandoutputstore/store.go store.Get -- 0.00% +0.00%
pkg/app/api/commandoutputstore/store.go store.Put -- 0.00% +0.00%
pkg/app/api/commandoutputstore/store.go dataPath -- 0.00% +0.00%
pkg/app/api/grpcapi/grpcapi.go getPiped -- 57.14% +57.14%
pkg/app/api/grpcapi/grpcapi.go getApplication -- 57.14% +57.14%
pkg/app/api/grpcapi/grpcapi.go listApplications -- 0.00% +0.00%
pkg/app/api/grpcapi/grpcapi.go getDeployment -- 57.14% +57.14%
pkg/app/api/grpcapi/grpcapi.go getCommand -- 0.00% +0.00%
pkg/app/api/grpcapi/grpcapi.go addCommand -- 0.00% +0.00%
pkg/app/api/grpcapi/grpcapi.go makeGitPath -- 0.00% +0.00%
pkg/app/api/grpcapi/grpcapi.go listEnvironments -- 0.00% +0.00%
pkg/app/api/grpcapi/grpcapi.go getEnvironment -- 0.00% +0.00%
pkg/model/command.go Command.IsHandled -- 0.00% +0.00%
pkg/app/api/grpcapi/api.go API.GetPlanPreviewResults 0.00% 0.00% +0.00%
pkg/app/api/grpcapi/piped_api.go PipedAPI.ReportCommandHandled 0.00% 0.00% +0.00%
pkg/app/api/grpcapi/utils.go getPiped 57.14% -- -57.14%
pkg/app/api/grpcapi/utils.go getApplication 57.14% -- -57.14%
pkg/app/api/grpcapi/utils.go listApplications 0.00% -- +-0.00%
pkg/app/api/grpcapi/utils.go getDeployment 57.14% -- -57.14%
pkg/app/api/grpcapi/utils.go getCommand 0.00% -- +-0.00%
pkg/app/api/grpcapi/utils.go addCommand 0.00% -- +-0.00%
pkg/app/api/grpcapi/utils.go makeGitPath 0.00% -- +-0.00%
pkg/app/api/grpcapi/utils.go listEnvironments 0.00% -- +-0.00%
pkg/app/api/grpcapi/utils.go getEnvironment 0.00% -- +-0.00%

@khanhtc1202
Copy link
Member

/lgtm

if err != nil {
return nil, err
}

// TODO: Implement GetPlanPreviewResults RPC.
const freshDuration = 24 * time.Hour
Copy link
Member

Choose a reason for hiding this comment

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

Is it really needed to be in the block? I feel it should be out of there.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍
The command output contains the diff generated by our drift detector or external tools like terraform.
Of course, we are masking sensitive data while comparing but just in case.
So we can think of this as an additional safety guard.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, what I mean was this const freshDurations should be out of this function block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I misunderstood what you wanted to say.
I think we should put it inside that function to strict its scope. It must only be used by that function, no one else.

Copy link
Member

Choose a reason for hiding this comment

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

Fine, I got your point 👍

// Additional data to be stored in filestore.
bytes data = 5;
// Additional output data to be stored in filestore.
bytes output = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Okay, currently nothing uses the Data field 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@nakabonne
Copy link
Member

There you go.
/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by nakabonne.

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 c7f3d17 into master Jun 24, 2021
@pipecd-bot pipecd-bot deleted the command-output branch June 24, 2021 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants