Skip to content

Bug 1751147: Correct ca-bundle filename in terminate on change #195

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
Merged
merged 3 commits into from
Sep 17, 2019
Merged

Bug 1751147: Correct ca-bundle filename in terminate on change #195

merged 3 commits into from
Sep 17, 2019

Conversation

@mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Sep 16, 2019

No description provided.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 16, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 16, 2019

@mfojtik: This pull request references Bugzilla bug 1751147, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1751147: Correct ca-bundle filename in terminate on change

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 16, 2019
@mfojtik mfojtik force-pushed the terminate-on-trusted-ca branch 2 times, most recently from 446b8ed to 4a635c5 Compare September 16, 2019 12:14
@sttts
Copy link
Contributor

sttts commented Sep 16, 2019

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 16, 2019
@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2019

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 16, 2019
@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 16, 2019

Added glide bump to pickup an observer bug in library-go, testing locally.

sttts
sttts reviewed Sep 16, 2019
View reviewed changes
@mfojtik mfojtik force-pushed the terminate-on-trusted-ca branch from 0c033a4 to 8c6f292 Compare September 16, 2019 13:24
@sttts
Copy link
Contributor

sttts commented Sep 16, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2019
@mfojtik mfojtik force-pushed the terminate-on-trusted-ca branch from 8c6f292 to 314f8f2 Compare September 16, 2019 14:42
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2019
@sttts
Copy link
Contributor

sttts commented Sep 16, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2019
@stlaz
Copy link
Contributor

stlaz commented Sep 17, 2019

/hold
You still need something like https://github.com/openshift/cluster-authentication-operator/pull/194/files#diff-c37e62a9d7e53c40a57f44b1b9780494R534 to fix the BZ

@mfojtik mfojtik force-pushed the terminate-on-trusted-ca branch from 314f8f2 to a3cc4d5 Compare September 17, 2019 09:07
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2019
@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2019

@stlaz fixed in the last commit, PTAL

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2019

The file observer reaction to /var/run/configmaps/trusted-ca-bundle/ca-bundle.crt change was tested manually and the container successfully restart.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2019
stlaz
stlaz reviewed Sep 17, 2019
View reviewed changes
pkg/operator2/operator.go
rt, err := transportFor("", caData, nil, nil)
// if systemCABundle is not empty, append the new line to the caData
if len(c.systemCABundle) > 0 {
caData = append(bytes.TrimSpace(caData), []byte("\n")...)
Copy link
Contributor

@stlaz stlaz Sep 17, 2019

Choose a reason for hiding this comment

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

nit: no need to trim the spaces here, there can be multiple empty lines between the certs
https://goplay.space/#2tCqSuOBWJL

@stlaz
Copy link
Contributor

stlaz commented Sep 17, 2019

/lgtm
based on an offline discussion, I'm fine with the newline

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, stlaz, sttts

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@stlaz stlaz mentioned this pull request Sep 17, 2019
Closed
@openshift-merge-robot openshift-merge-robot merged commit 259d7a1 into openshift:master Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@enj enj Awaiting requested review from enj

2 more reviewers

@sttts sttts sttts left review comments

@stlaz stlaz stlaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@stlaz stlaz

@sttts sttts

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mfojtik @openshift-ci-robot @sttts @stlaz @openshift-merge-robot