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

[api] merge interpretation structs. #2732

Closed
wants to merge 1 commit into from

Conversation

ikaven1024
Copy link
Member

Signed-off-by: yingjinhui yingjinhui@didiglobal.com

What type of PR is this?

/kind api-change

What this PR does / why we need it:
xxxInterpretation structs can be merged.

Which issue(s) this PR fixes:
Part of #2371

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: yingjinhui <yingjinhui@didiglobal.com>
@karmada-bot karmada-bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 3, 2022
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kevin-wangzefeng after the PR has been reviewed.
You can assign the PR to them by writing /assign @kevin-wangzefeng in a comment when ready.

The full list of commands accepted by this bot can be found 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

@karmada-bot karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 3, 2022
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

/assign @RainbowMango

Comment on lines +57 to +74
// Now only supports Lua. Interpretation.LuaScript holds the Lua script that is used to interpret the dependencies of
// a specific resource.
// The script should implement a function as follows:
// retention:
// luaScript: >
// function GetDependencies(desiredObj)
// dependencies = {}
// if desiredObj.spec.serviceAccountName ~= "" and desiredObj.spec.serviceAccountName ~= "default" then
// dependency = {}
// dependency.apiVersion = "v1"
// dependency.kind = "ServiceAccount"
// dependency.name = desiredObj.spec.serviceAccountName
// dependency.namespace = desiredObj.namespace
// dependencies[0] = {}
// dependencies[0] = dependency
// end
// return dependencies
// end
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems not to match the current operation.

// +optional
Retention *LocalValueRetention `json:"retention,omitempty"`
Retention *Interpretation `json:"retention,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Will the degree of coupling be too high?

@RainbowMango
Copy link
Member

xxxInterpretation structs can be merged.

Why? What's the benefit?

@ikaven1024
Copy link
Member Author

xxxInterpretation structs can be merged.

Why? What's the benefit?

I can handle the script in one function.

https://github.com/ikaven1024/karmada/blob/609351cf6281e7da51f6f8e608b2a29d8776f47a/pkg/resourceinterpreter/configurableinterpreter/configurable.go#L154-L190

@RainbowMango
Copy link
Member

The reason why I didn't use a common struct to describe the rules is script isn't the only way to represent the rules.
Each interpreter operation may be described in another easier way instead of script. The idea didn't present at current APIs, but you can see it in the original proposal. Except the lua script, people can specify the rule by Json rule:

// Fields indicates the fields that should be retained.
	// Each field describes a field in JsonPath format.
	// +optional
	Fields []string `json:"fields,omitempty"`

	// RetentionLua holds the Lua script that is used to retain runtime values to the desired specification.
	// The script should implement a function just like:
	//
	// function Retain(desiredObj, runtimeObj)
	//     desiredObj.spec.fieldFoo = runtimeObj.spec.fieldFoo
	//     return desiredObj
	// end
	//
	// RetentionLua only holds the function body part, take the `desiredObj` and `runtimeObj` as the function parameters or
	// global parameters, and finally retains a retained specification.
	// +optional
	RetentionLua string `json:"retentionLua,omitempty"`

In other words, each interpreter operation may have different customization methods respectively. I'm afraid using a common struct would be very hard to do so.

@codecov-commenter
Copy link

Codecov Report

Merging #2732 (be9debd) into master (bcd7814) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2732      +/-   ##
==========================================
- Coverage   32.73%   32.73%   -0.01%     
==========================================
  Files         199      199              
  Lines       19401    19401              
==========================================
- Hits         6351     6350       -1     
- Misses      12696    12697       +1     
  Partials      354      354              
Flag Coverage Δ
unittests 32.73% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/search/proxy/store/util.go 93.56% <0.00%> (-0.59%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ikaven1024 ikaven1024 closed this Nov 3, 2022
@ikaven1024
Copy link
Member Author

The reason why I didn't use a common struct to describe the rules is script isn't the only way to represent the rules. Each interpreter operation may be described in another easier way instead of script. The idea didn't present at current APIs, but you can see it in the original proposal. Except the lua script, people can specify the rule by Json rule:

// Fields indicates the fields that should be retained.
	// Each field describes a field in JsonPath format.
	// +optional
	Fields []string `json:"fields,omitempty"`

	// RetentionLua holds the Lua script that is used to retain runtime values to the desired specification.
	// The script should implement a function just like:
	//
	// function Retain(desiredObj, runtimeObj)
	//     desiredObj.spec.fieldFoo = runtimeObj.spec.fieldFoo
	//     return desiredObj
	// end
	//
	// RetentionLua only holds the function body part, take the `desiredObj` and `runtimeObj` as the function parameters or
	// global parameters, and finally retains a retained specification.
	// +optional
	RetentionLua string `json:"retentionLua,omitempty"`

In other words, each interpreter operation may have different customization methods respectively. I'm afraid using a common struct would be very hard to do so.

OK, Keep the api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants