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
Added code for fields with dots(.) in their names work as needed #4591
Conversation
Welcome @annelausf! |
Hi @annelausf. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
please sign the CLA! |
api/filters/fieldspec/fieldspec.go
Outdated
@@ -5,10 +5,10 @@ package fieldspec | |||
|
|||
import ( | |||
"fmt" | |||
"sigs.k8s.io/kustomize/kyaml/pathsplitterutil" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the imports
@@ -18,7 +16,6 @@ type AnnotationsTransformerPlugin struct { | |||
FieldSpecs []types.FieldSpec `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't commit this file
@@ -1,7 +1,7 @@ | |||
// Copyright 2021 The Kubernetes Authors. | |||
// SPDX-License-Identifier: Apache-2.0 | |||
|
|||
package utils | |||
package pathsplitterutil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just call this package "utils", and put it under kyaml/utils
|
||
import ( | ||
. "sigs.k8s.io/kustomize/kyaml/pathsplitterutil" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the import order, this needs to go down with the github one
kyaml/yaml/rnode.go
Outdated
@@ -9,6 +9,7 @@ import ( | |||
"io/ioutil" | |||
"log" | |||
"regexp" | |||
"sigs.k8s.io/kustomize/kyaml/pathsplitterutil" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here
/ok-to-test |
kyaml/yaml/rnode_test.go
Outdated
@@ -2310,7 +2310,7 @@ func TestGetAnnotations(t *testing.T) { | |||
} | |||
|
|||
func TestGetFieldValueWithDot(t *testing.T) { | |||
t.Skip() | |||
//t.Skip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove this line, and the TODO
below
@annelausf: This PR has multiple commits, and the default merge method is: merge. 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. |
@@ -6,6 +6,7 @@ package replacement | |||
import ( | |||
"errors" | |||
"fmt" | |||
utils2 "sigs.k8s.io/kustomize/kyaml/utils" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix imports, and lets call this something more descriptive, maybe kyaml_utils
instead of utils2
Please sign the CLA! And looks like there are some test failures. If you can narrow down the cause maybe we can help provide guidance to a solution. |
Working on signing the CLA and will look into the test case failures in the meantime. |
Hi @natasha41575 looked into the errors that we saw and found this. Seems like this test: |
There are some other tests failing though, that I think are actually caused by this PR, though I don't know why they would be failing. If you have time to investigate the other failures, that would be very helpful. |
I tested
There are other tests however, that are failing. You can see those logs here: https://github.com/kubernetes-sigs/kustomize/runs/6084779608?check_suite_focus=true |
Waiting to see if the changes we just made will fix the other tests. |
You can see the same test failures in the other PR that attempted to work on this #4501, so I think investigation is needed. You can run |
Also I signed the CLA, however it is still saying Missing CLA Authorization. Is there a way to rerun that check? |
/check-cla If this doesn't work, make sure your email address that you used to sign the CLA is set on your commits. |
/check-cla |
just testing |
Co-authored-by: sarjamil <sjamil@salesforce.com>
Co-authored-by: sarjamil <sjamil@salesforce.com>
Co-authored-by: sarjamil <sjamil@salesforce.com>
Co-authored-by: sarjamil <sjamil@salesforce.com>
Co-authored-by: sarjamil <sjamil@salesforce.com>
Add co-author to PR. Co-authored-by: sarjamil <sjamil@salesforce.com>
Co-authored-by: sarjamil sjamil@salesforce.com
@natasha41575 Signed the CLA! Let me know if there are any further comments. Thanks! |
/ok-to-test |
/retest |
Thank you very much! /lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: annelausf, natasha41575 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 |
Fixes #4487