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.

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

rename pod injection #396

Merged
merged 1 commit into from
Feb 24, 2017
Merged

rename pod injection #396

merged 1 commit into from
Feb 24, 2017

Conversation

jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Feb 23, 2017

from #254 (comment)

we also need to decide on the api group

ping @pmorie @erictune @bgrant0607

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 23, 2017
@pmorie
Copy link
Member

pmorie commented Feb 24, 2017

There seem to be two issues to settle:

  1. The name of the API group
  2. The name of the resource

I agree that the 'policy' name should be about what is allowed to do a thing. By that measure, this is not a policy at all. I like 'settings' as a name for this type of resource. How about this:

  • API group: settings
  • Resource name: ServiceInjection

@pmorie
Copy link
Member

pmorie commented Feb 24, 2017

@derekwaynecarr
Copy link
Member

derekwaynecarr commented Feb 24, 2017 via email

@derekwaynecarr
Copy link
Member

settings is fine too but I think injection should be avoided in favor of defaults.

@derekwaynecarr
Copy link
Member

How about API group settings and resource name is PodServiceDefaults? I think having the noun associated with the resource it mutates on admission front and center will avoid user confusion. Having Service* kind of obfuscates that it impacts Pods and not Services.

@derekwaynecarr
Copy link
Member

derekwaynecarr commented Feb 24, 2017

saying kubectl get podservicedefaults is not the worst thing...

@pmorie
Copy link
Member

pmorie commented Feb 24, 2017

I can live with PodServiceDefaults.

How about you @jessfraz ?

@liggitt
Copy link
Member

liggitt commented Feb 24, 2017

Remember that groups get k8s.io suffix. kubectl will find specified resources via server-specified discovery order and do fuzzy matching on group names, so any of the following would work:

kubectl get podservicedefaults
kubectl get podservicedefaults.settings
kubectl get podservicedefaults.settings.k8s.io

@pmorie pmorie self-assigned this Feb 24, 2017
@bgrant0607
Copy link
Member

@jessfraz Sorry I wasn't clear. I'd like to nix "Service", "Injection", and "Policy".

I like "settings" for the name of the API group.

"Defaults" doesn't quite fit, since the values in the Pod don't gracefully override the values in this resource, according to the proposal.

PodPresets?

@derekwaynecarr Is there a reason you wouldn't add compute resources to this API?

@jessfraz
Copy link
Contributor Author

settings seems ok to me

is pod presets ok? i'll wait for status quo before doing find replace lol

@pwittrock
Copy link
Member

pwittrock commented Feb 24, 2017

@bgrant0607 WDYT of "Dependencies" instead of "Defaults"? This looks similar to Inversion of Control software design principles.

@pmorie
Copy link
Member

pmorie commented Feb 24, 2017

I think settings is best for the name of the group, and PodPreset is fine for the name of the resource. It's hard to find a perfect name that will be perfect forever; I think settings/PodPreset is plenty-good.

@jessfraz jessfraz changed the title rename pod injection to service injection rename pod injection Feb 24, 2017
@bgrant0607
Copy link
Member

+1 to settings/PodPreset.

@pwittrock While this is similar in some ways to dependency injection, the mechanism is (deliberately) more general purpose than that. For example, it could be used just to set an environment variable in a group of pods.

@jessfraz
Copy link
Contributor Author

updated

unversioned.TypeMeta
ObjectMeta

// +optional
Spec PodInjectionPolicySpec
Spec Podpresetpec
Copy link
Member

Choose a reason for hiding this comment

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

PodPresetSpec, plz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol

Signed-off-by: Jess Frazelle <acidburn@google.com>
@pmorie
Copy link
Member

pmorie commented Feb 24, 2017

LGTM - I'll give it 30 min and then merge if no one objects.

@pwittrock
Copy link
Member

SGTM

@derekwaynecarr
Copy link
Member

derekwaynecarr commented Feb 24, 2017 via email

@pmorie
Copy link
Member

pmorie commented Feb 24, 2017

Merging, thanks for tolerating the bikeshedding, @jessfraz

@pmorie pmorie merged commit 4938913 into kubernetes:master Feb 24, 2017
@jessfraz jessfraz deleted the rename-pip branch February 24, 2017 20:53
@pmorie
Copy link
Member

pmorie commented Feb 27, 2017

@smarterclayton did you want to change the name?

@smarterclayton
Copy link
Contributor

I'm trying to reconcile "settings" to how we'd present this in UIs and in relation to other existing groups. Not opposed to it, although as a group it might be too close to "extensions" (grab bag for my stuff). I'll squawk if I can come up with an objection.

@smarterclayton
Copy link
Contributor

smarterclayton commented Mar 20, 2017 via email

ruebenramirez pushed a commit to ruebenramirez/community that referenced this pull request Apr 22, 2017
shyamjvs pushed a commit to shyamjvs/community that referenced this pull request Sep 22, 2017
draft of resource management node features relnotes
MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants