Skip to content

Conversation

@khanhtc1202
Copy link
Member

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 33.79%. This pull request increases coverage by 0.01%.

File Function Base Head Diff
pkg/config/deployment.go PipelineStage.UnmarshalJSON 41.94% 44.07% +2.13%

}

func (e *deployExecutor) ensurePromote(_ context.Context) model.StageStatus {
return model.StageStatus_STAGE_FAILURE
Copy link
Member

Choose a reason for hiding this comment

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

Would it mean to be not supported yet but planned in the future? If so, it would be nice to have a clear comment and emit log like e.LogPersister.Error("This stage is not implemented yet")`

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, let's me add that log 👍

e.deploySource = ds
e.deployCfg = ds.DeploymentConfig.LambdaDeploymentSpec
if e.deployCfg == nil {
e.LogPersister.Errorf("Malformed deployment configuration: missiong LambdaDeploymentSpec")
Copy link
Member

Choose a reason for hiding this comment

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

typo: missing

@nghialv
Copy link
Member

nghialv commented Jan 7, 2021

/lgtm

@pipecd-bot pipecd-bot added the lgtm label Jan 7, 2021
@nakabonne
Copy link
Member

Looks not to addressed our comment, but my browser's fault?

@nakabonne
Copy link
Member

Just to confirm. I don't mean to hurry you.

@pipecd-bot pipecd-bot removed the lgtm label Jan 7, 2021
@nakabonne
Copy link
Member

/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.

@khanhtc1202
Copy link
Member Author

Looks not to addressed our comment, but my browser's fault?

sorry, not yet commit the change 🙏

@nghialv
Copy link
Member

nghialv commented Jan 7, 2021

No. I lgtm-ed because our comments just are nits. So you can /approve this PR once he fixed them.

@nakabonne
Copy link
Member

@nghialv I got it 👍

@khanhtc1202
Copy link
Member Author

btw, is it just me but even after I commit and push the changes, refresh github page not mark outdate to addressed comments 😢

@pipecd-bot pipecd-bot merged commit ef2ae17 into master Jan 7, 2021
@pipecd-bot pipecd-bot deleted the lambda-executor branch January 7, 2021 07:13
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.

5 participants