Fix reverse translation issue#205
Conversation
cmd/mesh/manifest-migrate.go
Outdated
|
|
||
| func valueFileFilter(path string) bool { | ||
| return filepath.Base(path) == "values.yaml" | ||
| return filepath.Ext(path) == YAMLSuffix && strings.HasPrefix(filepath.Base(path), "values") |
There was a problem hiding this comment.
i'm concerned this is not going to be robust. we're relying on a filename being of the form values*.yaml - is there a better way to check for this, like file position wrt. charts root?
There was a problem hiding this comment.
I checked the helm package implementation, it just check whether the file name equals "values.yaml", like what was implemented here before. That would work for installer files, but I am thinking the requirement should not be too strict because we have seen "values*.yaml" naming.
I am not sure whether it is necessary to validate the file schema or content here, instead I think it might be more clear to just give user message what files are being translated or not.
pkg/translate/translate_value.go
Outdated
| } | ||
|
|
||
| cuPath := newPS + ".cpu.targetAverageUtilization" | ||
| cuVal, found, err := name.GetFromTreePath(valueTree, util.ToYAMLPath(cuPath)) |
There was a problem hiding this comment.
the operation here should probably be in a helper function - it's too detailed to follow at this level.
There was a problem hiding this comment.
I think the translateHPASpec function is already helper function to handle special translation, and since each hpa setting has slightly different handling so is not easy to extract general ways from it. So I try to refactor the logic inside this function.
pkg/translate/translate_value.go
Outdated
| return err | ||
| } | ||
| log.Infof("path has value in helm Value.yaml tree, mapping to output path %s", outPath) | ||
| if err := tpath.WriteNode(cpSpecTree, util.ToYAMLPath(outPath+".scaleTargetRef"), st); err != nil { |
There was a problem hiding this comment.
overall, i think the code above has repeated code that could be refactored at least to use helper function(s), if not to use a for loop and passing a list of mappings to it.
There was a problem hiding this comment.
I would try to refactor and consolidate the repeating logic. But even that we can't really just do a simple mapping as there is some minor difference to handle different settings, only minReplica and maxReplicas are handled the exactly same way
pkg/translate/translate_value.go
Outdated
| asVal, found, err := name.GetFromTreePath(valueTree, util.ToYAMLPath(valPath)) | ||
| if found && err == nil { | ||
| log.Infof("path has value in helm Value.yaml tree, mapping to output path %s", outPath) | ||
| if key == ".cpu.targetAverageUtilization" { |
There was a problem hiding this comment.
rather than checking for a special key, it's better to move this outside the loop, then have both this and the loop code call the same helper function if possible.
There was a problem hiding this comment.
removed the special key out of the loop
7f7ba45 to
d5a7167
Compare
cmd/mesh/manifest-migrate.go
Outdated
| "github.com/ghodss/yaml" | ||
| "github.com/gogo/protobuf/jsonpb" | ||
| "github.com/spf13/cobra" | ||
| "path/filepath" |
There was a problem hiding this comment.
File is not goimports-ed (from goimports)
| "path/filepath" |
9f0df56 to
7529b3d
Compare
|
/cherrypick release-1.3 |
|
@ostromart: new pull request created: #220 DetailsIn response to this:
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. |
* Fix reverse translation issue * Address hpa spec issues * Refactor * Second version of refactor * Address comment * lint
* Eliminate vfsgen again. (#200) * Fix build commmand for installing go-bindata (#202) * Update makefile for proto related rules (#169) * Update makefile for proto related rules to work for new system * Address comments * Add doc gen * Upload updated pb.go files * Update common * Address comment * Merge two install sections into one (#203) * Merge two install sections into one * Change section title * Update to 2019-08-16 commonfiles (#207) Enable MacOS based container builds. Depends-On: istio/common-files#30 Depends-On: istio/tools#274 * support resource type for k8s mapping. (#206) * support resource type for k8s mapping. * apply comments. * Fix reverse translation issue (#205) * Fix reverse translation issue * Address hpa spec issues * Refactor * Second version of refactor * Address comment * lint * Changes to API for mesh config integration (#210) * Changes to API for mesh config integration * Review comments, add strategy * Add strategy to k8s settings * Fix unit tests * update doc. (#212) * update doc. * apply comments. * update link with recent source. * Clean up values (#214) * remove invalid and duplicated fields. (#217)
Uh oh!
There was an error while loading. Please reload this page.