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

fix(core): ensure that users are not warned of their success #9794

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

dogonthehorizon
Copy link
Member

The ternary as written would allow a singleton list containing a single
null to propagate throughout the render method of this warning
message. This would result in a menacing "warning: no reason provided"
message appearing on all stage executions.

The assignment now states that a singleton list will only be provided
iff message is non null and the messages array is empty.

The ternary as written would allow a singleton list containing a single
`null` to propagate throughout the render method of this warning
message. This would result in a menacing "warning: no reason provided"
message appearing on all stage executions.

The assignment now states that a singleton list will only be provided
iff `message` is non null and the `messages` array is empty.
Copy link
Member

@cristhian-castaneda cristhian-castaneda left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@link108 link108 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!

@dogonthehorizon dogonthehorizon added the ready to merge Reviewed and ready for merge label Jan 12, 2022
@mergify mergify bot merged commit 333677b into spinnaker:master Jan 12, 2022
@mattgogerly
Copy link
Member

Thanks and sorry for not catching this case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Reviewed and ready for merge target-release/1.28
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants