-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
preserve order and comments in edit #233
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Liujingfang1 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 |
This is ready for review. Please take a look. |
# some descriptions for the patches | ||
patches: | ||
- service.yaml | ||
- pod.yaml |
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.
will it preserve the comments on the items in the list ?
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.
No, it won't. Only the comments ahead the item is preserved.
# some descriptions for the patches
patches:
# this is service.yaml
- service.yaml
- pod.yaml
will be
# some descriptions for the patches
patches:
- service.yaml
- pod.yaml
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.
ok. Thanks.
Here are some limitations for this change:
|
pkg/commands/kustomizationfile.go
Outdated
path string | ||
fsys fs.FileSystem | ||
comments []fieldComment | ||
fields map[string]bool |
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.
fields
indicates if a field shows up in the original kustomization.yaml. We can change it to a better name.
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.
clever hack!
pkg/commands/kustomizationfile.go
Outdated
pattern := regexp.MustCompile("^(" + strings.Join(kustomizationFields, "|") + "):") | ||
var err error | ||
var line []byte | ||
for err == nil { |
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.
can this be structured more traditionally as:
line, err := buffer.ReadBytes('\n')
for err == nil {
doStuff()
line, err := buffer.ReadBytes('\n')
}
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.
Done
pkg/commands/kustomizationfile.go
Outdated
var line []byte | ||
for err == nil { | ||
line, err = buffer.ReadBytes('\n') | ||
if strings.HasPrefix(string(line), "#") { |
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.
seems like this could be tweaked to allow the comment block to accumulate (and preserve) blank lines
might also want to allow leading blanks since it seems harmless.
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.
Yes, we can tweak it. I will update this later.
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.
Added the tweaking. I also added a test case.
pkg/commands/kustomizationfile.go
Outdated
buffer := bytes.NewBuffer(content) | ||
var comments [][]byte | ||
incomments := false | ||
pattern := regexp.MustCompile("^(" + strings.Join(kustomizationFields, "|") + "):") |
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.
this is effectively a package constant.
please declare immediately after kustomizationFields, and call it recognizedFields
or something (pattern
is too generic, like string or number).
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.
sounds good.
pkg/commands/kustomizationfile.go
Outdated
func (mf *kustomizationFile) getFieldComments(content []byte) error { | ||
buffer := bytes.NewBuffer(content) | ||
var comments [][]byte | ||
incomments := false |
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.
inComments
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.
I found inComments
is not needed. Removed it.
pkg/commands/kustomizationfile.go
Outdated
@@ -30,9 +34,18 @@ import ( | |||
"github.com/kubernetes-sigs/kustomize/pkg/types" | |||
) | |||
|
|||
var kustomizationFields = []string{"resources", "bases", "namePrefix", "namespace", "crds", "commonLabels", "commonAnnotations", "patches", "configMapGenerator", "secretGenerator", "vars", "imageTags"} | |||
|
|||
type fieldComment struct { |
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 called this commentedField
, and add a comment to the type saying something like field
has to be a recognized kustomization field, but comment
can be empty.
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.
added.
pkg/commands/kustomizationfile.go
Outdated
fsys fs.FileSystem | ||
path string | ||
fsys fs.FileSystem | ||
comments []fieldComment |
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 call this originalFields
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.
or something. i.e. these fields were present in the original file, as opposed to ones added later.
pkg/commands/kustomizationfile.go
Outdated
path string | ||
fsys fs.FileSystem | ||
comments []fieldComment | ||
fields map[string]bool |
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.
isn't this redundant? i.e.
func (mf *kustomizationFile) hasField(name string) bool {
for _, n := range mf.originalFields {
if n.whatever == name {
return true
}
}
return false
}
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.
Yes, we can do this by a for loop. I use a map to avoid this loop :)
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.
two data structures instead of one introduces the possibility that they mean different things, and can get out of sync as code changes. performance not an issue here.
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.
done
pkg/commands/kustomizationfile.go
Outdated
@@ -100,3 +120,73 @@ func stringInSlice(str string, list []string) bool { | |||
} | |||
return false | |||
} | |||
|
|||
func (mf *kustomizationFile) getFieldComments(content []byte) error { |
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.
parseCommentedFields
(its not a getter)
pkg/commands/kustomizationfile.go
Outdated
} | ||
output = append(output, content...) | ||
} | ||
for field, found := range mf.fields { |
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.
this could range over kustomizationFields
and call hasField
pkg/commands/kustomizationfile.go
Outdated
if found { | ||
continue | ||
} | ||
content, err := marshalField(field, kustomization) |
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.
can u add a comment on what happens if the field wasn't in the original file and wasn't added?
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.
Sure, it returns an empty []byte. Will also add this comment in the PR.
Address comments and add change to preserve blank line. Aggregate inside field comments and move them to ahead of the field. Add unit test. @monopole PTAL |
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.
/lgtm
looks great, merging |
time to release? and address #236? |
I will add a deprecation message. Then do the release. |
Fix #17