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

Initial optional configmap proposal #175

Merged
merged 2 commits into from Dec 22, 2016

Conversation

Projects
None yet
5 participants
@fraenkel
Contributor

fraenkel commented Dec 14, 2016

Proposal for #170

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Dec 16, 2016

Member

LGTM

Member

thockin commented Dec 16, 2016

LGTM

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Dec 16, 2016

Member

I'll leave this open for a few days to collect feedback

Member

thockin commented Dec 16, 2016

I'll leave this open for a few days to collect feedback

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Dec 19, 2016

Member

Would like to keep the secret envFrom and volume spec consistent with whatever we do here

Member

liggitt commented Dec 19, 2016

Would like to keep the secret envFrom and volume spec consistent with whatever we do here

@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Dec 19, 2016

Contributor

Yes, the intent would be to apply to others. Until the initial EnvFrom support goes in followed by add ons for Secrets, then Optional can apply to other bits. But right now, Secrets aren't there so it's cart before the horse.

Contributor

fraenkel commented Dec 19, 2016

Yes, the intent would be to apply to others. Until the initial EnvFrom support goes in followed by add ons for Secrets, then Optional can apply to other bits. But right now, Secrets aren't there so it's cart before the horse.

protocol: TCP
- containerPort: 2380
protocol: TCP
envFrom:

This comment has been minimized.

@liggitt

liggitt Dec 19, 2016

Member

need to define the behavior of an envvar that is both defined literally and from an optional config map which doesn't exist

...
spec:
  containers:
  - name: etcd
    image: openshift/etcd-20-centos7
    env:
    - name: foo
      value: bar
    - name: foo
      valueFrom:
        configMapKeyRef:
          name: configMapA
          key: some-key
          optional: true
@liggitt

liggitt Dec 19, 2016

Member

need to define the behavior of an envvar that is both defined literally and from an optional config map which doesn't exist

...
spec:
  containers:
  - name: etcd
    image: openshift/etcd-20-centos7
    env:
    - name: foo
      value: bar
    - name: foo
      valueFrom:
        configMapKeyRef:
          name: configMapA
          key: some-key
          optional: true

This comment has been minimized.

@thockin

thockin Dec 22, 2016

Member

Agree. This is a great corner case. I think defining it as "unchanged from previous value" seems correct.

@thockin

thockin Dec 22, 2016

Member

Agree. This is a great corner case. I think defining it as "unchanged from previous value" seems correct.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Dec 19, 2016

Member

should also apply to env.valueFrom.[configMapKeyRef|secretKeyRef]

Member

liggitt commented Dec 19, 2016

should also apply to env.valueFrom.[configMapKeyRef|secretKeyRef]

@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Dec 19, 2016

Contributor

@liggitt Hmm, I hadn't thought of making configmaps optional everywhere. But from the consistency POV I can see the rationale.

Contributor

fraenkel commented Dec 19, 2016

@liggitt Hmm, I hadn't thought of making configmaps optional everywhere. But from the consistency POV I can see the rationale.

Show outdated Hide outdated contributors/design-proposals/optional-configmap.md
files to populate the volume. The ConfigMaps should be allowed to be optional.
The default behavior will fail the volume setup if the ConfigMap is missing.
If the ConfigMap is marked as optional, the volume will be set up with no
files, and continue.

This comment has been minimized.

@liggitt

liggitt Dec 19, 2016

Member

Currently, the files in the configmap volume are updated while the pod is running if the configmap changes.

Define what happens if an optional configmap is missing when the pod is started on the node, and the configmap appears later while the pod is running... will the files be populated at that point, or not?

@liggitt

liggitt Dec 19, 2016

Member

Currently, the files in the configmap volume are updated while the pod is running if the configmap changes.

Define what happens if an optional configmap is missing when the pod is started on the node, and the configmap appears later while the pod is running... will the files be populated at that point, or not?

Show outdated Hide outdated contributors/design-proposals/optional-configmap.md
also define an optional ConfigMap that is used to override the values. There
may be a set of these optional ConfigMaps that provide environment specific
overrides for a given test cluster or region cluster. If no overrides are
present, the container will get the default values.

This comment has been minimized.

@liggitt

liggitt Dec 19, 2016

Member

the container will get the default values

Not sure what "default values" means here. Is this the "define literally, then define optional configmap overrides" case I asked about below?

@liggitt

liggitt Dec 19, 2016

Member

the container will get the default values

Not sure what "default values" means here. Is this the "define literally, then define optional configmap overrides" case I asked about below?

@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Dec 19, 2016

Contributor

@liggitt Hopefully this helps a bit more.

Contributor

fraenkel commented Dec 19, 2016

@liggitt Hopefully this helps a bit more.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Dec 19, 2016

Member

thanks

Member

liggitt commented Dec 19, 2016

thanks

@thockin

LGTM

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Dec 22, 2016

Member

Let's get this feature implemented!

@MrHohn I know you want this...

Member

thockin commented Dec 22, 2016

Let's get this feature implemented!

@MrHohn I know you want this...

@thockin thockin merged commit c9f881b into kubernetes:master Dec 22, 2016

1 check passed

cla/linuxfoundation fraenkel authorized
Details

fraenkel added a commit to fraenkel/kubernetes.github.io that referenced this pull request Jan 24, 2017

chenopis added a commit to kubernetes/website that referenced this pull request Jan 24, 2017

@fruch

This comment has been minimized.

Show comment
Hide comment
@fruch

fruch Jan 30, 2017

This is now mentioned in the offical documention, but https://kubernetes.io/docs/api-reference/v1/definitions/ doesn't show them at all

fruch commented Jan 30, 2017

This is now mentioned in the offical documention, but https://kubernetes.io/docs/api-reference/v1/definitions/ doesn't show them at all

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jan 30, 2017

Member

that site publishes docs for the current release, which is still 1.5.x

Member

liggitt commented Jan 30, 2017

that site publishes docs for the current release, which is still 1.5.x

@fruch

This comment has been minimized.

Show comment
Hide comment
@fruch

fruch Jan 31, 2017

I find it very confusing that the official ConfigMap docs says something different than current realease. I would expect official documentation to mention which version introduced what feature clearly. I was supprized that optional didn't worked for, when trying to follow documentation

fruch commented Jan 31, 2017

I find it very confusing that the official ConfigMap docs says something different than current realease. I would expect official documentation to mention which version introduced what feature clearly. I was supprized that optional didn't worked for, when trying to follow documentation

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jan 31, 2017

Member

What official docs are you referring to?

Member

liggitt commented Jan 31, 2017

What official docs are you referring to?

@fruch

This comment has been minimized.

Show comment
Hide comment
@fruch

This comment has been minimized.

Show comment
Hide comment
@fruch

fruch Jan 31, 2017

I.e. the first hit when looking for ConfigMap...

fruch commented Jan 31, 2017

I.e. the first hit when looking for ConfigMap...

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jan 31, 2017

Member

@fraenkel @jaredbhatti looks like the doc PR merged to master prematurely. Can we back those docs out and open the PR for the 1.6 doc branch instead?

Member

liggitt commented Jan 31, 2017

@fraenkel @jaredbhatti looks like the doc PR merged to master prematurely. Can we back those docs out and open the PR for the 1.6 doc branch instead?

@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Jan 31, 2017

Contributor

Someone needs to create a release-1.6 branch.

Contributor

fraenkel commented Jan 31, 2017

Someone needs to create a release-1.6 branch.

@thockin

This comment has been minimized.

Show comment
Hide comment
Member

thockin commented Feb 1, 2017

@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Feb 17, 2017

Contributor

So how do we revert this?
I can rebase on the 1.6 branch now that it is there.

Contributor

fraenkel commented Feb 17, 2017

So how do we revert this?
I can rebase on the 1.6 branch now that it is there.

@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Feb 17, 2017

Contributor

I will send a PR to revert.

Contributor

fraenkel commented Feb 17, 2017

I will send a PR to revert.

ruebenramirez pushed a commit to ruebenramirez/community that referenced this pull request Apr 22, 2017

Initial optional configmap proposal (#175)
* Initial optional configmap proposal

* Clarify Optional for ConfigMap and Secrets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment