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

Resolve admission plugin config files #12321

Merged
merged 4 commits into from
Jan 16, 2018
Merged

Resolve admission plugin config files #12321

merged 4 commits into from
Jan 16, 2018

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Dec 21, 2016 •
edited
Loading

  • moves server config to testdata files
  • applies defaults to test configs
  • fixes admission plugin config file locations to resolve the same way all other file references in the config files are resolved

@liggitt
Copy link
Contributor Author

liggitt commented Dec 21, 2016

@deads2k PTAL

@liggitt
Copy link
Contributor Author

liggitt commented Dec 21, 2016

hmm, complaining about taking the address of a field in a struct value

@deads2k
Copy link
Contributor

deads2k commented Dec 21, 2016

lgtm

@deads2k
Copy link
Contributor

deads2k commented Dec 21, 2016

Any ideas on how to keep from slipping? Make people opt-out on new fields?

@liggitt
Copy link
Contributor Author

liggitt commented Dec 21, 2016 •
edited
Loading

updated.

first commit is a straight move of the server config test data into standalone files, second commit round-trips to apply defaults to it

deads2k
deads2k reviewed Dec 22, 2016
View reviewed changes
pkg/cmd/server/api/types.go Outdated
@@ -1233,7 +1233,7 @@ type LDAPQuery struct {
type AdmissionPluginConfig struct {
// Location is the path to a configuration file that contains the plugin's
// configuration
Location string
Location *string
Copy link
Contributor

Choose a reason for hiding this comment

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

so strange.

deads2k
deads2k reviewed Dec 22, 2016
View reviewed changes
pkg/cmd/server/api/v1/conversions.go Outdated
@@ -219,6 +219,15 @@ func addDefaultingFuncs(scheme *runtime.Scheme) error {
obj.ServiceAccountMethod = "prompt"
}
},
func(obj *AdmissionConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this live in conversion and not in defaulting?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this live in conversion and not in defaulting?

Put another way, even if we didn't convert this, you'd still want to end up with non-nil values in the external object, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is defaulting, the defaulting functions just got added in the conversions file. I can move them all if we want to

Copy link
Contributor

Choose a reason for hiding this comment

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

This is defaulting, the defaulting functions just got added in the conversions file. I can move them all if we want to

Ah, well that explains it. I don't feel strongly about a move.

@deads2k
Copy link
Contributor

deads2k commented Dec 22, 2016

On question about where the defaulting happens, lgtm either way.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 20, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 28, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 9, 2017
@liggitt liggitt reopened this Jun 19, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 1, 2017
@openshift-merge-robot openshift-merge-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 24, 2017
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2017
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 28, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2018
@liggitt
Copy link
Contributor Author

liggitt commented Jan 13, 2018

wow, that year passed quick

@liggitt liggitt added this to the 3.9.0 milestone Jan 13, 2018
@liggitt liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Jan 13, 2018
@openshift openshift deleted a comment from openshift-bot Jan 15, 2018
@openshift openshift deleted a comment from openshift-bot Jan 15, 2018
@deads2k
Copy link
Contributor

deads2k commented Jan 15, 2018

/lgtm

longest time in lgtm state ever?

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

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

@liggitt
Copy link
Contributor Author

liggitt commented Jan 15, 2018

/retest

@openshift openshift deleted a comment from openshift-bot Jan 15, 2018
@openshift openshift deleted a comment from openshift-bot Jan 15, 2018
@openshift openshift deleted a comment from openshift-bot Jan 15, 2018
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 15, 2018 •
edited
Loading

@liggitt: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_builds bdfc2e1 link /test extended_builds

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.

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.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 9a9a948 into openshift:master Jan 16, 2018
@liggitt liggitt deleted the resolve-plugin-config-files branch January 20, 2018 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@mfojtik mfojtik

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. needs-api-review size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v3.9
Development

Successfully merging this pull request may close these issues.
7 participants
@liggitt @deads2k @openshift-ci-robot @openshift-merge-robot @mfojtik @smarterclayton @openshift-bot