[WIP] [FlyteCTL] enable kubectl-like configuration from multiple files #6292

machichima · 2025-03-01T08:32:32Z

Tracking issue

Why are the changes needed?

flytectl update workflow-execution-config can only accept one --attrFile at a time now, which is not convenient especially over multiple projects / domains.

What changes were proposed in this pull request?

Three different configuration ways are added (analogous to how kubectl works):

Setting multiple config files with multiple --attrFile (e.g. flytectl update workflow-execution-config --attrFile file1.yaml --attrFile file2.yaml
Pass a directory path to --attrFile (e.g. flytectl update workflow-execution-config --attrFile folder/

How was this patch tested?

Labels

Please add one or more of the following labels to categorize your PR:

added: For new features.
changed: For changes in existing functionality.
deprecated: For soon-to-be-removed features.
removed: For features being removed.
fixed: For any bug fixed.
security: In case of vulnerabilities

This is important to improve the readability of release notes.

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

Summary by Bito

This PR enhances flytectl's workflow execution configuration by enabling multiple attribute file configurations, similar to kubectl. It refactors the configuration structure and command logic to handle file lists instead of single files, simplifies the update process, and improves error messaging. The changes include revised flag definitions, updated data types, and improved test cases.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 4

Signed-off-by: machichima <[email protected]>

…like-config Signed-off-by: machichima <[email protected]>

flyte-bot · 2025-03-01T08:32:58Z

Code Review Agent Run #38bf67

Actionable Suggestions - 1

flytectl/cmd/config/subcommand/workflowexecutionconfig/attrupdateconfig_flags.go - 1
- Type mismatch between flag and struct definition · Line 53-53

Review Details

Files reviewed - 5 · Commit Range: b8a0b09..2f38c33
- flytectl/cmd/config/subcommand/workflowexecutionconfig/attrupdateconfig_flags.go
- flytectl/cmd/config/subcommand/workflowexecutionconfig/update_config.go
- flytectl/cmd/testutils/test_utils.go
- flytectl/cmd/update/matchable_workflow_execution_config.go
- flytectl/cmd/update/matchable_workflow_execution_config_test.go
Files skipped - 0
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

codecov · 2025-03-01T08:35:35Z

Codecov Report

Attention: Patch coverage is 15.90909% with 74 lines in your changes missing coverage. Please review.

Project coverage is 58.42%. Comparing base (ff7e79f) to head (62def7b).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
flytectl/cmd/update/update.go	15.68%	43 Missing ⚠️
flytectl/cmd/config/attrupdateconfig_flags.go	22.22%	21 Missing ⚠️
flytectl/cmd/testutils/test_utils.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #6292       +/-   ##
===========================================
+ Coverage   33.82%   58.42%   +24.60%     
===========================================
  Files        1329      938      -391     
  Lines      147808    71150    -76658     
===========================================
- Hits        49992    41571     -8421     
+ Misses      92974    26427    -66547     
+ Partials     4842     3152     -1690

Flag	Coverage Δ
unittests-datacatalog	`59.06% <ø> (+11.05%)`	⬆️
unittests-flyteadmin	`56.30% <ø> (+6.20%)`	⬆️
unittests-flytecopilot	`30.99% <ø> (ø)`
unittests-flytectl	`63.98% <15.90%> (+5.89%)`	⬆️
unittests-flyteidl	`76.12% <ø> (+69.33%)`	⬆️
unittests-flyteplugins	`61.00% <ø> (+11.98%)`	⬆️
unittests-flytepropeller	`54.79% <ø> (+18.26%)`	⬆️
unittests-flytestdlib	`64.04% <ø> (+13.66%)`	⬆️

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.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

flyte-bot · 2025-03-01T08:36:05Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
New Feature - New Flag Configuration Implementation	- `attrupdateconfig_flags.go` - Introduces new flag configuration for attribute update handling. - `update_config.go` - Defines the AttrUpdateConfig structure to support multiple attribute files.
Testing - Enhanced Flag Configuration Tests	- `attrupdateconfig_flags_test.go` - Contains tests verifying flag set behaviors and proper config decoding. - `test_utils.go` - Adds utility for verifying specific log counts to support improved test validations.
Other Improvements - Miscellaneous Code Commentary and Minor Updates	- `updater.go` - Updates comment to reflect usage of the new mockery-v2 tool. - `matchable_workflow_execution_config.go` - Adds a comment clarifying the processing of each attribute file.
Feature Improvement - Refactored Update Command for Multi-file Support	- `update.go` - Refactors command creation and execution logic to support multiple attribute files with enhanced parsing and subcommand execution.

flyte-bot · 2025-03-01T08:36:07Z

flytectl/cmd/config/subcommand/workflowexecutionconfig/attrupdateconfig_flags.go

@@ -50,7 +50,7 @@ func (AttrUpdateConfig) mustMarshalJSON(v json.Marshaler) string {
 // flags is json-name.json-sub-name... etc.
 func (cfg AttrUpdateConfig) GetPFlagSet(prefix string) *pflag.FlagSet {
 	cmdFlags := pflag.NewFlagSet("AttrUpdateConfig", pflag.ExitOnError)
-	cmdFlags.StringVar(&DefaultUpdateConfig.AttrFile, fmt.Sprintf("%v%v", prefix, "attrFile"), DefaultUpdateConfig.AttrFile, "attribute file name to be used for updating attribute for the resource type.")
+    cmdFlags.StringSliceVar(&DefaultUpdateConfig.AttrFile, fmt.Sprintf("%v%v", prefix, "attrFile"), DefaultUpdateConfig.AttrFile, "attribute file name to be used for updating attribute for the resource type.")


Type mismatch between flag and struct definition

The type of AttrFile is being changed from string to []string (via StringSliceVar), but the struct definition in the related files still defines it as string. This will cause type mismatch issues when accessing this field.

Code suggestion

Check the AI-generated fix before applying

Suggested change

cmdFlags.StringSliceVar(&DefaultUpdateConfig.AttrFile, fmt.Sprintf("%v%v", prefix, "attrFile"), DefaultUpdateConfig.AttrFile, "attribute file name to be used for updating attribute for the resource type.")

cmdFlags.StringVar(&DefaultUpdateConfig.AttrFile, fmt.Sprintf("%v%v", prefix, "attrFile"), DefaultUpdateConfig.AttrFile, "attribute file name to be used for updating attribute for the resource type.")

Code Review Run #38bf67

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

Signed-off-by: machichima <[email protected]>

flyte-bot · 2025-03-01T10:35:27Z

Code Review Agent Run #7699d3

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: 2f38c33..f43ae50
- flytectl/cmd/config/subcommand/workflowexecutionconfig/attrupdateconfig_flags.go
- flytectl/cmd/config/subcommand/workflowexecutionconfig/attrupdateconfig_flags_test.go
Files skipped - 0
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

Signed-off-by: machichima <[email protected]>

flyte-bot · 2025-03-10T15:13:13Z

Code Review Agent Run #988f93

Actionable Suggestions - 1

flytectl/cmd/config/attrupdateconfig_flags.go - 1
- Possible pointer dereferencing issue in elemValueOrNil · Line 22-22

Review Details

Files reviewed - 10 · Commit Range: f43ae50..62def7b
- flytectl/cmd/config/attrupdateconfig_flags.go
- flytectl/cmd/config/attrupdateconfig_flags_test.go
- flytectl/cmd/config/subcommand/workflowexecutionconfig/attrupdateconfig_flags.go
- flytectl/cmd/config/subcommand/workflowexecutionconfig/attrupdateconfig_flags_test.go
- flytectl/cmd/config/subcommand/workflowexecutionconfig/update_config.go
- flytectl/cmd/config/update_config.go
- flytectl/cmd/update/interfaces/updater.go
- flytectl/cmd/update/matchable_workflow_execution_config.go
- flytectl/cmd/update/matchable_workflow_execution_config_test.go
- flytectl/cmd/update/update.go
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-03-10T15:25:00Z

flytectl/cmd/config/attrupdateconfig_flags.go

+		if reflect.ValueOf(v).IsNil() {
+			return reflect.Zero(t.Elem()).Interface()
+		} else {
+			return reflect.ValueOf(v).Interface()


Possible pointer dereferencing issue in elemValueOrNil

The else branch in elemValueOrNil returns reflect.ValueOf(v).Interface() which doesn't dereference the pointer. This will return the pointer itself rather than the value it points to. Consider using reflect.ValueOf(v).Elem().Interface() to properly dereference the pointer.

Code suggestion

Check the AI-generated fix before applying

Suggested change

return reflect.ValueOf(v).Interface()

return reflect.ValueOf(v).Elem().Interface()

Code Review Run #988f93

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

machichima added 3 commits March 1, 2025 16:09

feat: enable passing multiple attrFile flag

b8a0b09

Signed-off-by: machichima <[email protected]>

test: for multple --attrFile in workflow-execution-config

e5774ec

Signed-off-by: machichima <[email protected]>

Merge branch 'master' of github.com:flyteorg/flyte into 6237-kubectl-…

2f38c33

…like-config Signed-off-by: machichima <[email protected]>

flyte-bot reviewed Mar 1, 2025

View reviewed changes

fix: run make generate

f43ae50

Signed-off-by: machichima <[email protected]>

machichima added 4 commits March 10, 2025 21:47

feat: pass config in yaml to subcommands

4eb2189

Signed-off-by: machichima <[email protected]>

fix: revert modification in matchable workflow execution

a0311b5

Signed-off-by: machichima <[email protected]>

fix: revert matchable workflow config

171443e

Signed-off-by: machichima <[email protected]>

fix: goimports

62def7b

Signed-off-by: machichima <[email protected]>

flyte-bot reviewed Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [FlyteCTL] enable kubectl-like configuration from multiple files #6292

[WIP] [FlyteCTL] enable kubectl-like configuration from multiple files #6292

machichima commented Mar 1, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Mar 1, 2025 •

edited

Loading

Code Review Agent Run #38bf67

codecov bot commented Mar 1, 2025 •

edited

Loading

flyte-bot commented Mar 1, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Mar 1, 2025

flyte-bot commented Mar 1, 2025 •

edited

Loading

Code Review Agent Run #7699d3

flyte-bot commented Mar 10, 2025 •

edited

Loading

Code Review Agent Run #988f93

flyte-bot Mar 10, 2025

	cmdFlags.StringSliceVar(&DefaultUpdateConfig.AttrFile, fmt.Sprintf("%v%v", prefix, "attrFile"), DefaultUpdateConfig.AttrFile, "attribute file name to be used for updating attribute for the resource type.")
	cmdFlags.StringVar(&DefaultUpdateConfig.AttrFile, fmt.Sprintf("%v%v", prefix, "attrFile"), DefaultUpdateConfig.AttrFile, "attribute file name to be used for updating attribute for the resource type.")

	return reflect.ValueOf(v).Interface()
	return reflect.ValueOf(v).Elem().Interface()

[WIP] [FlyteCTL] enable kubectl-like configuration from multiple files #6292

Are you sure you want to change the base?

[WIP] [FlyteCTL] enable kubectl-like configuration from multiple files #6292

Conversation

machichima commented Mar 1, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Labels

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Summary by Bito

flyte-bot commented Mar 1, 2025 • edited Loading

Code Review Agent Run #38bf67

codecov bot commented Mar 1, 2025 • edited Loading

Codecov Report

flyte-bot commented Mar 1, 2025 • edited Loading

Changelist by Bito

flyte-bot Mar 1, 2025

Choose a reason for hiding this comment

flyte-bot commented Mar 1, 2025 • edited Loading

Code Review Agent Run #7699d3

flyte-bot commented Mar 10, 2025 • edited Loading

Code Review Agent Run #988f93

flyte-bot Mar 10, 2025

Choose a reason for hiding this comment

machichima commented Mar 1, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Mar 1, 2025 •

edited

Loading

codecov bot commented Mar 1, 2025 •

edited

Loading

flyte-bot commented Mar 1, 2025 •

edited

Loading

flyte-bot commented Mar 1, 2025 •

edited

Loading

flyte-bot commented Mar 10, 2025 •

edited

Loading