-
Notifications
You must be signed in to change notification settings - Fork 210
Make pipectl plan-preview command renders the results to stdout and file #2154
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,14 +44,16 @@ type API struct { | |
| commandStore commandstore.Store | ||
| commandOutputGetter commandOutputGetter | ||
|
|
||
| logger *zap.Logger | ||
| webBaseURL string | ||
| logger *zap.Logger | ||
| } | ||
|
|
||
| // NewAPI creates a new API instance. | ||
| func NewAPI( | ||
| ds datastore.DataStore, | ||
| cmds commandstore.Store, | ||
| cog commandOutputGetter, | ||
| webBaseURL string, | ||
| logger *zap.Logger, | ||
| ) *API { | ||
| a := &API{ | ||
|
|
@@ -62,6 +64,7 @@ func NewAPI( | |
| eventStore: datastore.NewEventStore(ds), | ||
| commandStore: cmds, | ||
| commandOutputGetter: cog, | ||
| webBaseURL: webBaseURL, | ||
| logger: logger.Named("api"), | ||
| } | ||
| return a | ||
|
|
@@ -437,7 +440,20 @@ func (a *API) GetPlanPreviewResults(ctx context.Context, req *apiservice.GetPlan | |
| return nil, err | ||
| } | ||
|
|
||
| const freshDuration = 24 * time.Hour | ||
| const ( | ||
| freshDuration = 24 * time.Hour | ||
| defaultCommandHandleTimeout = 5 * time.Minute | ||
| ) | ||
|
|
||
| var ( | ||
| handledCommands = make([]string, 0, len(req.Commands)) | ||
| results = make([]*model.PlanPreviewCommandResult, 0, len(req.Commands)) | ||
| ) | ||
|
|
||
| commandHandleTimeout := time.Duration(req.CommandHandleTimeout) * time.Second | ||
| if commandHandleTimeout == 0 { | ||
| commandHandleTimeout = defaultCommandHandleTimeout | ||
| } | ||
|
|
||
| // Validate based on command model stored in datastore. | ||
| for _, commandID := range req.Commands { | ||
|
|
@@ -456,24 +472,35 @@ func (a *API) GetPlanPreviewResults(ctx context.Context, req *apiservice.GetPlan | |
| if cmd.Type != model.Command_BUILD_PLAN_PREVIEW { | ||
| return nil, status.Error(codes.FailedPrecondition, fmt.Sprint("Command %s is not a plan preview command", commandID)) | ||
| } | ||
|
|
||
| if !cmd.IsHandled() { | ||
| return nil, status.Error(codes.FailedPrecondition, fmt.Sprintf("Command %s is not completed yet", commandID)) | ||
| if time.Since(time.Unix(cmd.CreatedAt, 0)) <= commandHandleTimeout { | ||
| return nil, status.Error(codes.NotFound, fmt.Sprintf("No command ouput for command %d because it is not completed yet", commandID)) | ||
| } | ||
| results = append(results, &model.PlanPreviewCommandResult{ | ||
| CommandId: cmd.Id, | ||
| PipedId: cmd.PipedId, | ||
| Error: fmt.Sprintf("Timed out, maybe the Piped is offline currently."), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| // There is no reason to fetch output data of command that has been completed a long time ago. | ||
| // So in order to prevent unintended actions, we disallow that ability. | ||
| if time.Since(time.Unix(cmd.HandledAt, 0)) > freshDuration { | ||
| return nil, status.Error(codes.FailedPrecondition, fmt.Sprintf("The output data for command %s is too old for access", commandID)) | ||
| } | ||
| } | ||
|
|
||
| results := make([]*model.PlanPreviewCommandResult, 0, len(req.Commands)) | ||
| handledCommands = append(handledCommands, commandID) | ||
| } | ||
|
|
||
| // Fetch ouput data to build results. | ||
| for _, commandID := range req.Commands { | ||
| for _, commandID := range handledCommands { | ||
| data, err := a.commandOutputGetter.Get(ctx, commandID) | ||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, fmt.Sprintf("Failed to retrieve output data of command %s", commandID)) | ||
| } | ||
|
|
||
| var result model.PlanPreviewCommandResult | ||
| if err := json.Unmarshal(data, &result); err != nil { | ||
| a.logger.Error("failed to unmarshal planpreview command result", | ||
|
|
@@ -482,9 +509,16 @@ func (a *API) GetPlanPreviewResults(ctx context.Context, req *apiservice.GetPlan | |
| ) | ||
| return nil, status.Error(codes.Internal, fmt.Sprintf("Failed to decode output data of command %s", commandID)) | ||
| } | ||
|
|
||
| results = append(results, &result) | ||
| } | ||
|
|
||
| // All URL fields inside the result model are empty. | ||
| // So we fill them before sending to the client. | ||
| for _, r := range results { | ||
| r.FillURLs(a.webBaseURL) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I thought putting this inside right after the loop is enough (like you did before commit 😄 ). But if you intend to be more clear, it's on you
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will keep this as it is because there are 2 loops (L480, L512) that are updating this list.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is true 👍 |
||
| } | ||
|
|
||
| return &apiservice.GetPlanPreviewResultsResponse{ | ||
| Results: results, | ||
| }, nil | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.