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.

Sign up for GitHub

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

Jump to bottom

Fix bug in container lifecycle event message generation #56959

Merged
merged 1 commit into from
Dec 12, 2017
Merged

Fix bug in container lifecycle event message generation #56959

merged 1 commit into from
Dec 12, 2017

Conversation

lichuqiang
Copy link
Contributor

@lichuqiang lichuqiang commented Dec 8, 2017 •
edited
Loading

What this PR does / why we need it:
In HandlerRunner of container lifecycle, the event msg is re-declared. Thus, the event message we returned would always be empty.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #56962

Special notes for your reviewer:
/sig node
Release note:

Fix bug in container lifecycle event messaging

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 8, 2017
@yujuhong
Copy link
Contributor

yujuhong commented Dec 8, 2017

/assign
Looks good but need to change the unit test to check the error (as opposed to the msg)
E.g.,

if err != expectedErr {

@yujuhong yujuhong added the kind/bug Categorizes issue or PR as related to a bug. label Dec 8, 2017
@yujuhong
Copy link
Contributor

yujuhong commented Dec 8, 2017

Can you also add a release note? This is worth cherry-picking to other branches. Thanks!

@yujuhong yujuhong added this to the v1.9 milestone Dec 8, 2017
@lichuqiang
Copy link
Contributor Author

Looks good but need to change the unit test to check the error (as opposed to the msg)

Yes, will update the test case, thanks :)

@jberkus jberkus mentioned this pull request Dec 10, 2017
Closed
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 11, 2017
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 11, 2017
@lichuqiang
Copy link
Contributor Author

@yujuhong updated :)

@dims
Copy link
Member

dims commented Dec 11, 2017

/assign @dchen1107

@dims
Copy link
Member

dims commented Dec 11, 2017

@kubernetes/sig-node-pr-reviews Please take a look

@yujuhong
Copy link
Contributor

Thanks!

/lgtm

I think this can go in 1.9, but if it's too late, 1.9.1 would be fine too.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2017
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lichuqiang, yujuhong

Associated issue: #56962

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2017
@yujuhong yujuhong added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 11, 2017
@yujuhong
Copy link
Contributor

@lichuqiang could you also cherry-pick this to 1.8 and 1.7?

@jberkus
Copy link

jberkus commented Dec 11, 2017

@yujuhong hey, the label is "status/approved-for-milestone". Please fix that and the submit queue will do the rest.

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Needs Attention

@Random-Liu @dchen1107 @lichuqiang @vishh @yujuhong @kubernetes/sig-node-misc

Action required: During code freeze, pull requests in the milestone should be in progress.
If this pull request is not being actively worked on, please remove it from the milestone.
If it is being worked on, please add the status/in-progress label so it can be tracked with other in-flight pull requests.

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required
Pull Request Labels
  • sig/node: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help

This was referenced Dec 12, 2017
Merged
Merged
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 56599, 56824, 56918, 56967, 56959). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit b97e176 into kubernetes:master Dec 12, 2017
k8s-github-robot pushed a commit that referenced this pull request Dec 15, 2017
Merge pull request #57064 from lichuqiang/release-1.7
d0e7e11 
Automatic merge from submit-queue.

Automated cherry pick of #56959

Cherry pick of #56959 on release-1.7.
#56959: Fix bug in container lifecycle event messaging

/assign @yujuhong
jpbetz added a commit that referenced this pull request Dec 20, 2017
@jpbetz
Merge pull request #57063 from lichuqiang/release-1.8
6260bb0 
Automated cherry pick of #56959
@lovejoy lovejoy mentioned this pull request Jun 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Random-Liu Random-Liu

@vishh vishh

@dchen1107 dchen1107

@yujuhong yujuhong

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/needs-attention priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

Message in container lifecycle event(poststart/prestop) always empty
10 participants
@lichuqiang @yujuhong @dims @k8s-ci-robot @jberkus @k8s-github-robot @Random-Liu @enisoc @vishh @dchen1107