Skip to content

Start observing the Proxy object#530

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
stlaz:proxy_observing
Aug 2, 2019
Merged

Start observing the Proxy object#530
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
stlaz:proxy_observing

Conversation

@stlaz
Copy link
Copy Markdown
Contributor

@stlaz stlaz commented Jul 24, 2019

Adds an observer for the Proxy object, pulls its fields and passes
them to the environment of the deployed kube-apiserver container.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2019
@stlaz
Copy link
Copy Markdown
Contributor Author

stlaz commented Jul 25, 2019

/retest

@stlaz stlaz force-pushed the proxy_observing branch from 9af5a4a to 2f483fe Compare July 25, 2019 11:24
@mfojtik
Copy link
Copy Markdown
Contributor

mfojtik commented Jul 25, 2019 •
edited
Loading

I think this code is generic enough to live in library-go and being re-used by other operators.

(can be a follow up)

mfojtik
mfojtik reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/configobservation/proxy/observe_proxy.go Outdated
)

func ObserveProxyConfig(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (map[string]interface{}, []error) {
const proxyPath = "proxy"
Copy link
Copy Markdown
Contributor

@mfojtik mfojtik Jul 25, 2019

Choose a reason for hiding this comment

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

move this outside, make a comment describing what this is.

Copy link
Copy Markdown
Contributor

@sttts sttts Jul 25, 2019

Choose a reason for hiding this comment

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

Can we instead share this path somehow between config observation and target config controller? Would make it clear that these two are interconnected.

mfojtik
mfojtik reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/configobservation/proxy/observe_proxy.go
mfojtik
mfojtik reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/configobservation/proxy/observe_proxy.go Outdated
mfojtik
mfojtik reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/configobservation/proxy/observe_proxy.go
mfojtik
mfojtik reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/targetconfigcontroller/targetconfigcontroller.go Outdated
sttts
sttts reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/targetconfigcontroller/targetconfigcontroller.go Outdated
sttts
sttts reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/targetconfigcontroller/targetconfigcontroller.go Outdated
sttts
sttts reviewed Jul 25, 2019
View reviewed changes
Comment thread pkg/operator/configobservation/proxy/observe_proxy.go Outdated
@stlaz
Copy link
Copy Markdown
Contributor Author

stlaz commented Jul 25, 2019

@mfojtik was right to request the observer in library-go, here be the PR: openshift/library-go#485

@stlaz stlaz force-pushed the proxy_observing branch from 2f483fe to f957446 Compare July 26, 2019 10:19
@stlaz
Copy link
Copy Markdown
Contributor Author

stlaz commented Jul 26, 2019

This now uses the not-yet merged code from lib-go, appears to work

@stlaz stlaz force-pushed the proxy_observing branch from f957446 to 32a3b4e Compare July 29, 2019 07:50
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2019
@stlaz
Copy link
Copy Markdown
Contributor Author

stlaz commented Jul 29, 2019

Pulled in the latest library-go to get the proxy observer from there

@stlaz stlaz force-pushed the proxy_observing branch from 32a3b4e to d6f078f Compare July 29, 2019 13:05
@stlaz
Copy link
Copy Markdown
Contributor Author

stlaz commented Jul 29, 2019

Grabbed the MergePrunedConfigMap() method from library-go to auto-prune the fields of observed config that don't belong to the config object.

@stlaz stlaz force-pushed the proxy_observing branch 2 times, most recently from 8bcdc38 to c04aae6 Compare July 30, 2019 11:37
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2019
@stlaz stlaz force-pushed the proxy_observing branch from c04aae6 to 16f8efa Compare August 2, 2019 08:15
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 2, 2019
stlaz added 2 commits August 2, 2019 14:02
@stlaz
bump (*)
5471d8f
@stlaz
Start observing the Proxy object
abbf9e5 
Adds an observer for the Proxy object, pulls its fields and passes
them to the environment of the deployed kube-apiserver container.
@stlaz stlaz force-pushed the proxy_observing branch from 16f8efa to abbf9e5 Compare August 2, 2019 12:03
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2019
@sttts
Copy link
Copy Markdown
Contributor

sttts commented Aug 2, 2019

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2019
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Aug 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2019
@openshift-merge-robot openshift-merge-robot merged commit a3239f8 into openshift:master Aug 2, 2019
@deads2k deads2k mentioned this pull request Sep 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soltysh soltysh Awaiting requested review from soltysh

2 more reviewers

@mfojtik mfojtik mfojtik left review comments

@sttts sttts sttts left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sttts sttts

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

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