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

enhancement: ignore field support slice type #243

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

howieyuen
Copy link
Collaborator

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #241

Special notes for your reviewer:

Slice type passes tests, use recursive type assertion can do that.
kind can not be specified, because ResourceNode supports k8s and tf, Type field is used to differ them.

Does this PR introduce a user-facing change?

enhancement: ignore field support slice type

Additional documentation e.g., design docs, usage docs, etc.:

None

@coveralls
Copy link

coveralls commented Feb 13, 2023

Pull Request Test Coverage Report for Build 4159591163

  • 18 of 20 (90.0%) changed or added relevant lines in 1 file are covered.
  • 31 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 71.967%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/engine/operation/graph/resource_node.go 18 20 90.0%
Files with Coverage Reduction New Missed Lines %
pkg/engine/operation/graph/resource_node.go 31 60.5%
Totals Coverage Status
Change from base Build 4144172590: 0.08%
Covered Lines: 4644
Relevant Lines: 6453

💛 - Coveralls

Copy link
Contributor

@healthjyk healthjyk left a comment

Choose a reason for hiding this comment

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

1, add the option to specify the "type" may be better, more flexible;
2, if removeNestedField failed, why no error returned?
3, removeNestedField cannot handle nested field with type which is not map[string]interface{} or []interface{}, it's better to give clear indication of this func is only used for obj generated by json unmarshal, and show the process of json marshal and unmarshal in the test case.

@howieyuen
Copy link
Collaborator Author

howieyuen commented Feb 13, 2023

1, add the option to specify the "type" may be better, more flexible; 2, if removeNestedField failed, why no error returned? 3, removeNestedField cannot handle nested field with type which is not map[string]interface{} or []interface{}, it's better to give clear indication of this func is only used for obj generated by json unmarshal, and show the process of json marshal and unmarshal in the test case.

  1. the reason I wrote in the issue comment
  2. remove nested field is inspired from k8s.io/apimachinery(pkg/apis/meta/v1/unstructured/helpers.go), it's a helper function, the only error, not such field, should not abort the apply process.
  3. it's not for json marshall/unmarshall, generated manifests contain the user intent, this func is only use for ignored fields during preview step.

@healthjyk
Copy link
Contributor

I saw the issue already, included the "type" is more useful for me.

@howieyuen
Copy link
Collaborator Author

I saw the issue already, included the "type" is more useful for me.

I'll try it again, see is there any better idea to make it.

@howieyuen howieyuen merged commit 7a6e482 into KusionStack:main Feb 13, 2023
@howieyuen howieyuen deleted the ignore-fields branch February 13, 2023 12:54
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kusion prevew flag --ignore-fields support more than map[string]interface{} type
3 participants