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

feat: refactor config-related api #7773

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

sophon-zt
Copy link
Contributor

No description provided.

@sophon-zt sophon-zt linked an issue Jul 10, 2024 that may be closed by this pull request
@sophon-zt sophon-zt marked this pull request as draft July 10, 2024 08:09
@github-actions github-actions bot added the size/XXL Denotes a PR that changes 1000+ lines. label Jul 10, 2024
@sophon-zt sophon-zt force-pushed the feature/improvement-refactor-configconstraint-api-issue7751 branch from 8a19366 to b180fca Compare August 2, 2024 03:31
@sophon-zt sophon-zt force-pushed the feature/improvement-refactor-configconstraint-api-issue7751 branch from b180fca to b912e2b Compare August 9, 2024 07:43
// +kubebuilder:validation:Required
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:Pattern:=`^[a-z0-9]([a-z0-9\.\-]*[a-z0-9])?$`
Name string `json:"name"`
Copy link
Contributor

@weicao weicao Aug 10, 2024

Choose a reason for hiding this comment

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

There's no need to bind parameters to config files. Instead, we should define a list of parameters (key-value pairs) for a Component in the ComponentDefinition, along with specifying how modifications to these parameters take effect. This way, users can set these parameters for the Component directly without specifying the config files in which they reside.

This approach is preferable for two reasons:

  • It reduces the burden on users by eliminating the need to specify config files when setting parameters.
  • Some parameters don't appear in any config files at all.

// The ParametersDefinition object defines the format of the configuration file and all parameters type.
//
// +optional
ParametersDef string `json:"parametersDefName,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

ParametersDef should be taken out and decoupled from ConfigTemplate, as some parameters are not present in configuration files.

You can try to place ParametersDef under ComponentDefinition directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

If some parameters reside in Config Templates, they should be identified and referenced in the ParametersDef.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I want to express is that ParameterDef should not be a supplementary description of the Config Template ConfigMap.

Instead, it should be a list of parameters that are allowed to be set for a Component. If modifying one of these parameters requires updating a configuration file, then this configuration file should be referenced in the description of that parameter.

@sophon-zt sophon-zt force-pushed the feature/improvement-refactor-configconstraint-api-issue7751 branch from b912e2b to 0cee5a4 Compare August 30, 2024 13:01
@sophon-zt sophon-zt force-pushed the feature/improvement-refactor-configconstraint-api-issue7751 branch from 23c53ff to 4703634 Compare September 20, 2024 07:45
@sophon-zt sophon-zt marked this pull request as ready for review September 20, 2024 07:47
@sophon-zt sophon-zt requested review from zjx20, a team and leon-inf as code owners September 20, 2024 07:47
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 70.79208% with 177 lines in your changes missing coverage. Please review.

Project coverage is 60.80%. Comparing base (240bbf8) to head (9606118).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controllerutil/parameter_schema.go 0.00% 47 Missing ⚠️
...kg/controller/configuration/configuration_utils.go 73.63% 20 Missing and 9 partials ⚠️
...s/configuration/parametersdefinition_controller.go 72.22% 18 Missing and 7 partials ⚠️
...ollers/apps/transformer_component_configuration.go 72.72% 11 Missing and 7 partials ⚠️
pkg/controller/configuration/pipeline.go 67.64% 8 Missing and 3 partials ⚠️
controllers/configuration/parameter_controller.go 38.46% 8 Missing ⚠️
...ers/configuration/componentparameter_controller.go 84.09% 5 Missing and 2 partials ⚠️
pkg/controller/builder/builder_component.go 0.00% 6 Missing ⚠️
pkg/controller/configuration/template_wrapper.go 60.00% 3 Missing and 3 partials ⚠️
...ps/transformer_component_parameters_reconfigure.go 92.45% 3 Missing and 1 partial ⚠️
... and 9 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7773      +/-   ##
==========================================
- Coverage   61.17%   60.80%   -0.37%     
==========================================
  Files         358      360       +2     
  Lines       41282    41464     +182     
==========================================
- Hits        25254    25212      -42     
- Misses      13766    14006     +240     
+ Partials     2262     2246      -16     
Flag Coverage Δ
unittests 60.80% <70.79%> (-0.37%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sophon-zt sophon-zt force-pushed the feature/improvement-refactor-configconstraint-api-issue7751 branch from 4703634 to 851ce9a Compare September 20, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] refactor configconstraint api
2 participants