Skip to content

Add support for default key in operator boilerplate #65

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
Closed
enj wants to merge 3 commits into from
Closed

Add support for default key in operator boilerplate #65

enj wants to merge 3 commits into from

Conversation

@enj
Copy link
Contributor

@enj enj commented Feb 9, 2019 •
edited
Loading

Add support for default key in operator boilerplate

This change allows us to support flows where the key of the operator
does not exist (i.e. in case it is deleted).

It also adds generic support for defaulting the key.

Signed-off-by: Monis Khan mkhan@redhat.com

Add defaulting for operatorv1.Authentication

This change makes it so that we render an empty
operatorv1.Authentication on start. This is then defaulted to the
managed state at runtime. A missing Authentication resource is
treated as the same as an empty one.

The defaults are stored once in the defaultCopyAuthenticationFunc
function (they are not duplicated in any manifests). They are also
applied regardless of if the resource exists in the API (any unset
field means "give me the default").

Signed-off-by: Monis Khan mkhan@redhat.com

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2019
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 9, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2019
@stlaz
Copy link
Contributor

stlaz commented Feb 11, 2019

/retest

@stlaz
Copy link
Contributor

stlaz commented Feb 11, 2019

/lgtm

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

openshift-ci-robot commented Feb 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@stlaz
Copy link
Contributor

stlaz commented Feb 11, 2019

We don't necessary need to remove defaultOperatorConfig, I'm fine with us creating it on start if it's not present.

The only trouble I'm seeing is that now we have the constant default defined in two different places in the code, but meh.

@ericavonb
Copy link
Contributor

ericavonb commented Feb 18, 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 Feb 18, 2019
@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 18, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@enj
Copy link
Contributor Author

enj commented Feb 18, 2019

/hold

I do not think we need it for beta2 and I want to mull on the shape of WithDefaultKey for a bit longer.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2019
@stlaz stlaz mentioned this pull request Feb 28, 2019
Closed
@enj enj force-pushed the enj/i/default_key branch from c855fb1 to 094be9c Compare February 28, 2019 17:50
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 28, 2019

New changes are detected. LGTM label has been removed.

@enj
Copy link
Contributor Author

enj commented Feb 28, 2019

@stlaz take another look. This is the direction I am thinking for #86

enj added 2 commits February 28, 2019 16:10
@enj
Add support for default key in operator boilerplate
7e29fc0 
This change allows us to support flows where the key of the operator
does not exist (i.e. in case it is deleted).

It also adds generic support for defaulting the key.

Signed-off-by: Monis Khan <mkhan@redhat.com>
@enj
Add defaulting for operatorv1.Authentication
c10c8b4 
This change makes it so that we render an empty
operatorv1.Authentication on start.  This is then defaulted to the
managed state at runtime.  A missing Authentication resource is
treated as the same as an empty one.

The defaults are stored once in the defaultCopyAuthenticationFunc
function (they are not duplicated in any manifests).  They are also
applied regardless of if the resource exists in the API (any unset
field means "give me the default").

Signed-off-by: Monis Khan <mkhan@redhat.com>
@enj enj force-pushed the enj/i/default_key branch from 094be9c to c10c8b4 Compare February 28, 2019 21:11
@enj
Copy link
Contributor Author

enj commented Feb 28, 2019

Tested locally. Handles missing key as expected.

@enj
Copy link
Contributor Author

enj commented Feb 28, 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 Feb 28, 2019
@enj
Copy link
Contributor Author

enj commented Mar 1, 2019

/retest

@stlaz
Copy link
Contributor

stlaz commented Mar 1, 2019

The code looks good, yet the logs of console suggest once again that it can't find the oauth well-known endpoint:

2019/03/1 12:47:25 auth: error contacting auth provider (retrying in 2s): discovery through endpoint https://172.30.0.1:443/.well-known/oauth-authorization-server failed: 404 Not Found
2019/03/1 12:47:27 auth: error contacting auth provider (retrying in 4s): discovery through endpoint https://172.30.0.1:443/.well-known/oauth-authorization-server failed: 404 Not Found
2019/03/1 12:47:31 auth: error contacting auth provider (retrying in 8s): discovery through endpoint https://172.30.0.1:443/.well-known/oauth-authorization-server failed: 404 Not Found
2019/03/1 12:47:39 auth: error contacting auth provider (retrying in 16s): discovery through endpoint https://172.30.0.1:443/.well-known/oauth-authorization-server failed: 404 Not Found

@enj
hack: log everything
1e0dde0 
Signed-off-by: Monis Khan <mkhan@redhat.com>
@enj
Copy link
Contributor Author

enj commented Mar 1, 2019

/hold

I see the issue. Will address next week.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 3, 2019

@enj: PR needs rebase.

Details

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 needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 28, 2019

@enj: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws link /test e2e-aws
ci/prow/e2e-aws-operator link /test e2e-aws-operator
ci/prow/e2e-aws-upgrade 1e0dde0 link /test e2e-aws-upgrade
ci/prow/e2e-aws-console-login 1e0dde0 link /test e2e-aws-console-login
ci/prow/verify 1e0dde0 link /test verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@enj
Copy link
Contributor Author

enj commented Oct 9, 2019

/close

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 9, 2019

@enj: Closed this PR.

Details

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrogers950 mrogers950 Awaiting requested review from mrogers950

@stlaz stlaz Awaiting requested review from stlaz

Assignees

@stlaz stlaz

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@enj @stlaz @openshift-ci-robot @ericavonb @openshift-bot