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

Add K_NAMESPACE env var to user-container #7086

Closed
wants to merge 1 commit into from

Conversation

savitaashture
Copy link
Contributor

Fixes #6947

Proposed Changes

  • Add K_NAMESPACE env to user-container

Release Note

None

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 28, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 28, 2020
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@savitaashture: 0 warnings.

In response to this:

Fixes #6947

Proposed Changes

  • Add K_NAMESPACE env to user-container

Release Note

None

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.

@knative-prow-robot knative-prow-robot added area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Feb 28, 2020
@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-integration-tests pull-knative-serving-integration-tests 1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: savitaashture
To complete the pull request process, please assign dprotaso
You can assign the PR to them by writing /assign @dprotaso in a comment when ready.

The full list of commands accepted by this bot can be found 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

@@ -32,6 +32,7 @@ type ResourceNames struct {
Route string
Revision string
Service string
Namespace string
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any assertions of this field in the tests

Copy link
Member

Choose a reason for hiding this comment

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

So I'm curious if you need/want this

@dgerd
Copy link

dgerd commented Feb 28, 2020

Related to #4190

Personally I would rather see namespace offered through the downward API shape than introduce it as another K_ variable. It has the benefit of letting the user specify the EnvVar they expect in their application making it more portable. metadata.namespace is the same on the Service/Configuration as it is on the Pod making the fieldRef for this particular use-case of fieldRef fairly natural.

cc @mattmoor

@vagababov
Copy link
Contributor

also
/cc @evankanderson

@mattmoor
Copy link
Member

Yeah, we should be solving this via the downward API

@duglin
Copy link

duglin commented Feb 29, 2020 via email

@mattmoor
Copy link
Member

@duglin This PR is the tip of the iceberg. To provide the relief you want, we need to update specs / docs as well, or this isn't discoverable. Honestly, I don't think the work involved to solve #4190 would be significantly larger than the work to properly land this.

@duglin
Copy link

duglin commented Feb 29, 2020

Maybe, but #4190 has been there since May

@mattmoor
Copy link
Member

So help us find someone to drive that work...? :)

@vagababov
Copy link
Contributor

Can we close this PR, since it seems it'll be done as part of #4190.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2020
@knative-prow-robot
Copy link
Contributor

@savitaashture: PR needs rebase.

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.

@vagababov
Copy link
Contributor

I am going to close it, since we closed the underlying bug.
/close

@knative-prow-robot
Copy link
Contributor

@vagababov: Closed this PR.

In response to this:

I am going to close it, since we closed the underlying bug.
/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
Labels
area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add K_NAMESPACE env var to pods
9 participants