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

fix --local panic in set commands #65636

Merged

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Jun 29, 2018

Release note:

Fixed panic when performing a "set env" operation on a --local resource

In progress - identifying other commands with this issue

cc @soltysh

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 29, 2018
@juanvallejo juanvallejo changed the title fix --local panic "set env" [WIP] fix --local panic "set env" Jun 29, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2018
@juanvallejo juanvallejo changed the title [WIP] fix --local panic "set env" fix --local panic in set commands Jun 29, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 29, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/fix-local-panic branch 2 times, most recently from de28d7d to c821f1d Compare June 29, 2018 15:05
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", 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 Jun 29, 2018
@soltysh soltysh added cherrypick-candidate and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 29, 2018
@@ -471,7 +471,7 @@ func (o *EnvOptions) RunEnv() error {
for _, patch := range patches {
info := patch.Info
if patch.Err != nil {
allErrs = append(allErrs, fmt.Errorf("error: %s/%s %v\n", info.Mapping.Resource, info.Name, patch.Err))
allErrs = append(allErrs, fmt.Errorf("error: %s/%s %v\n", info.Object.GetObjectKind().GroupVersionKind().Kind, info.Name, patch.Err))
Copy link
Member

Choose a reason for hiding this comment

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

kind is not the same as resource... this changes the output here

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we were panicing on --local before (hence no output to change under that code path), I can go ahead and leave info.Mapping.Resource as it was, and only output kind under --local. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a reasonable middle ground, eventually we can entirely skip that part of the message with --local

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can do the same thing we do for -o name without resource info... lower(<kind>[.<group>])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, PR updated

@juanvallejo
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed 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. labels Jun 29, 2018
@idealhack
Copy link
Member

/test pull-kubernetes-e2e-gce

@juanvallejo
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@juanvallejo
Copy link
Contributor Author

@liggitt friendly ping

@@ -597,6 +597,19 @@ func ManualStrip(file []byte) []byte {
return stripped
}

// GetObjectName receives a resource Info and returns an approximate
// form of the resource's kind/name.
func GetObjectName(info *resource.Info) string {
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to make this a method on resource.Info, similar to it's existing String() method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll open a followup to address that.

@soltysh
Copy link
Contributor

soltysh commented Aug 15, 2018

@juanvallejo please address Jordan's comments.

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. and removed cherrypick-candidate labels Sep 21, 2018
@nikhita
Copy link
Member

nikhita commented Sep 25, 2018

/sig cli
/kind bug

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. kind/bug Categorizes issue or PR as related to a bug. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 25, 2018
@juanvallejo
Copy link
Contributor Author

@soltysh

please address Jordan's comments.

Sorry about my delay here. Will find some time to wrap this PR up this week

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2018
@soltysh
Copy link
Contributor

soltysh commented Jan 15, 2019

@juanvallejo friendly ping

@juanvallejo
Copy link
Contributor Author

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2019
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@@ -597,6 +597,19 @@ func ManualStrip(file []byte) []byte {
return stripped
}

// GetObjectName receives a resource Info and returns an approximate
// form of the resource's kind/name.
func GetObjectName(info *resource.Info) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll open a followup to address that.

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, soltysh

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 lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 836ce9f into kubernetes:master Feb 8, 2019
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. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. 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.

7 participants