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

envtest: export DefaultKubeAPIServerFlags & make flags configurable #165

Merged
merged 1 commit into from
Nov 7, 2018
Merged

envtest: export DefaultKubeAPIServerFlags & make flags configurable #165

merged 1 commit into from
Nov 7, 2018

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Sep 30, 2018 •
edited

This change exports DefaultKubeAPIServerFlags, allowing a user to
append the default flags with other custom flags and set it to
Environment.KubeAPIServerFlags.
If KubeAPIServerFlags is not set, DefaultKubeAPIServerFlags is used when
the API server starts.

Fixes #112

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 30, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 30, 2018
@DirectXMan12
Copy link
Contributor

I'd prefer we didn't break backwards compat here, since it should be fairly easy to work around it. Perhaps just make DefaultAPIServerArgs public, and then default APIServerFlags if it's not set already?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 14, 2018
@darkowlzz darkowlzz changed the title envtest: append flags to defaultKubeAPIServerFlags envtest: export DefaultKubeAPIServerFlags & make flags configurable Oct 14, 2018
@darkowlzz
Copy link
Contributor Author

darkowlzz commented Oct 14, 2018 •
edited

I'd prefer we didn't break backwards compat here, since it should be fairly easy to work around it. Perhaps just make DefaultAPIServerArgs public, and then default APIServerFlags if it's not set already?

Yeah, that's much better. Removed the constructor and exported DefaultKubeAPIServerFlags. Since it's an exported slice now, users can append it on their own and set a custom set of flags to Environment.KubeAPIServerFlags.
Thanks!

droot
droot reviewed Oct 16, 2018
View reviewed changes
pkg/envtest/server.go
// TODO: create test framework interface to append flag to default flags.
var defaultKubeAPIServerFlags = []string{
// DefaultKubeAPIServerFlags are default flags necessary to bring up apiserver.
var DefaultKubeAPIServerFlags = []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to export this anymore given that we have made KubeAPIServerFlag configurable (change below) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, because we still want to allow people to make use of the default args if they just want to append.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification.

@DirectXMan12
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: darkowlzz, DirectXMan12

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2018
@darkowlzz
Copy link
Contributor Author

Rebased.

@darkowlzz
Copy link
Contributor Author

darkowlzz commented Oct 25, 2018 •
edited

Cyclomatic Complexity went above 10 because of the if statement in Environment.Start().
I created a new function Environment.getAPIServerFlags() to return the set flags or the default flags if not set. That fixed the failure due to CC > 10.
Hope that's the right way to handle it :)

@DirectXMan12
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2018
@DirectXMan12
Copy link
Contributor

@darkowlzz can you rebase, please (or let us know if you need us to step in and rebase)

@darkowlzz
envtest: export DefaultKubeAPIServerFlags & make it configurable
d0ead81 
This change exports DefaultKubeAPIServerFlags, allowing a user to
append the default flags with other custom flags and set it to
Environment.KubeAPIServerFlags.
If KubeAPIServerFlags is not set, DefaultKubeAPIServerFlags is used when
the API server starts.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2018
@darkowlzz
Copy link
Contributor Author

Rebased!

@DirectXMan12
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018
@k8s-ci-robot k8s-ci-robot merged commit 81f67a0 into kubernetes-sigs:master Nov 7, 2018
@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Nov 7, 2018 •
edited

Thanks!

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018
@k8s-ci-robot
Merge pull request kubernetes-sigs#165 from darkowlzz/112-AppendKubeA…
3ca478d 
…PIServerFlags

envtest: export DefaultKubeAPIServerFlags & make flags configurable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droot droot droot left review comments

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

Assignees

@DirectXMan12 DirectXMan12

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@darkowlzz @DirectXMan12 @k8s-ci-robot @droot