Skip to content

Show envFrom when running describe service or revision#630

Merged
knative-prow-robot merged 1 commit intoknative:masterfrom
MIBc:envfrom
Feb 2, 2020
Merged

Show envFrom when running describe service or revision#630
knative-prow-robot merged 1 commit intoknative:masterfrom
MIBc:envfrom

Conversation

@MIBc
Copy link
Copy Markdown
Contributor

@MIBc MIBc commented Feb 1, 2020

Fixes #626

Proposed Changes

  • Show envFrom when running describe revison. The result like:
    EnvFrom: ConfigMap:(testConfig), Secret:(testSecret)

Release Note:

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 1, 2020
@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 1, 2020
@knative-prow-robot
Copy link
Copy Markdown
Contributor

Hi @MIBc. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 1, 2020
Copy link
Copy Markdown
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks a lot. I have one request for change of the output, but otherwise looks good.

Some other remarks:

  • Could you please add the changelog line directly into CHANGELOG.adoc and add it to the PR (as explained in the issue template) ?
  • Ideally we could already resolve the configmap or secret to the values they refer to, but I'm afraid that's not possible as we are no "allowed" to access configmaps/secrets via the K8s API. So having only the reference here is fine.

As a next step, we have to add this feature to kn service describe, too, for the currently active configuration, but also for all revisions (when use with --verbose)

Comment thread pkg/kn/commands/revision/describe.go Outdated
}
}
if len(configMap) > 0 {
result = append(result, fmt.Sprintf("ConfigMap:(%s)", strings.Join(configMap, ",")))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we please use the same format as it can be specified on the command line ? Ie. "cm:mymap" an "secret:mysecret" ? This helps to keep our wording consistent. (and you can better reuse it with copy&paste actions).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK.

@MIBc MIBc changed the title Show envFrom when running describe revision Show envFrom when running describe service or revision Feb 1, 2020
Copy link
Copy Markdown
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

thanks !

/lgtm

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 1, 2020
@rhuss
Copy link
Copy Markdown
Contributor

rhuss commented Feb 1, 2020

@MIBc there is a minor formatting glitch which causes the test to fail. Could please fix that, then we are good to merge. Thank you.

@MIBc
Copy link
Copy Markdown
Contributor Author

MIBc commented Feb 2, 2020

OK

Fixes knative#626

Show envFrom when running describe service or revison. The result like:
EnvFrom:  cm:test1, cm:test2
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2020
@knative-prow-robot
Copy link
Copy Markdown
Contributor

@MIBc: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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.

@navidshaikh
Copy link
Copy Markdown
Contributor

navidshaikh commented Feb 2, 2020 via email

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 2, 2020
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/commands/revision/describe.go 61.6% 63.8% 2.2
pkg/kn/commands/service/describe.go 79.6% 79.8% 0.1

@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/commands/revision/describe.go 61.6% 63.8% 2.2
pkg/kn/commands/service/describe.go 79.6% 79.8% 0.1

Copy link
Copy Markdown
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

thx !

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2020
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MIBc, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@knative-prow-robot knative-prow-robot merged commit d82d557 into knative:master Feb 2, 2020
@MIBc MIBc deleted the envfrom branch February 2, 2020 13:42
markusthoemmes added a commit to markusthoemmes/knative-client that referenced this pull request Mar 8, 2021
dsimansk added a commit to dsimansk/client that referenced this pull request Apr 9, 2025
Run `make generate-release` using openshift-knative/hack.

Co-authored-by: dsimansk <dsimansk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--env-from is not shown in service describe and revision describe

6 participants