[FlyteCTL] enable kubectl-like configuration from multiple files #6292
Conversation
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
…like-config Signed-off-by: machichima <[email protected]>
Code Review Agent Run #38bf67Actionable Suggestions - 1
Review Details
AI Code Review powered by |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6292 +/- ##
===========================================
+ Coverage 33.82% 58.44% +24.62%
===========================================
Files 1329 938 -391
Lines 147808 71200 -76608
===========================================
- Hits 49992 41614 -8378
+ Misses 92974 26418 -66556
+ Partials 4842 3168 -1674
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changelist by BitoThis pull request implements the following key changes.
|
| 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.") |
There was a problem hiding this comment.
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
| 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]>
Code Review Agent Run #7699d3Actionable Suggestions - 0Review Details
AI Code Review powered by |
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #988f93Actionable Suggestions - 1
Review Details
AI Code Review powered by |
| if reflect.ValueOf(v).IsNil() { | ||
| return reflect.Zero(t.Elem()).Interface() | ||
| } else { | ||
| return reflect.ValueOf(v).Interface() |
There was a problem hiding this comment.
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
| 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
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #ea5294Actionable Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
|
| } else if *clusterresourceattribute.DefaultAttrFileConfig == clustrResourceAttrFileConfig { | ||
| return fmt.Errorf("attrFile is mandatory while calling update for cluster resource attribute") |
There was a problem hiding this comment.
The condition *clusterresourceattribute.DefaultAttrFileConfig == clustrResourceAttrFileConfig is checking if the attribute file config is empty, but this comparison might not work as expected for structs. Consider using a more explicit check for empty fields instead.
Code suggestion
Check the AI-generated fix before applying
| } else if *clusterresourceattribute.DefaultAttrFileConfig == clustrResourceAttrFileConfig { | |
| return fmt.Errorf("attrFile is mandatory while calling update for cluster resource attribute") | |
| } else if clustrResourceAttrFileConfig.Project == "" && clustrResourceAttrFileConfig.Domain == "" && clustrResourceAttrFileConfig.Workflow == "" { | |
| return fmt.Errorf("attrFile is mandatory while calling update for cluster resource attribute") |
Code Review Run #ea5294
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #c58d0bActionable Suggestions - 0Review Details
|
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #85577cActionable Suggestions - 0Review Details
|
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #d139b4Actionable Suggestions - 0Review Details
|
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
machichima
changed the title
Code Review Agent Run #66e219Actionable Suggestions - 19
Additional Suggestions - 10
Review Details
|
| manager: conda | ||
| platform: linux-64 | ||
| dependencies: | ||
| botocore: '>=1.36.18,<1.37.0' | ||
| botocore: '>=1.37.13,<1.38.0' | ||
| jmespath: '>=0.7.1,<2.0.0' | ||
| python: '>=3.9' | ||
| s3transfer: '>=0.11.0,<0.12.0' |
There was a problem hiding this comment.
The dependency packaging: '>=16.8' has been replaced with pyyaml: '>=3.10'. However, looking at the updated code context, both dependencies are needed as they appear together in other sections (e.g., lines 2402 and 2426). This change might have been unintentional and could cause issues if the packaging dependency is required at this location.
Code suggestion
Check the AI-generated fix before applying
| s3transfer: '>=0.11.0,<0.12.0' | |
| packaging: '>=16.8' | |
| pyyaml: '>=3.10' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| numpy: '>=1.17' | ||
| pyarrow: '>=8.0.0' | ||
| python: '>=3.8.0' | ||
| requests: '>=2.19.0' |
There was a problem hiding this comment.
The dependency requirement for requests is being downgraded from >=2.32.2 to >=2.19.0. This older version might have security vulnerabilities that have been fixed in newer versions. Consider maintaining the requirement for the newer version.
Code suggestion
Check the AI-generated fix before applying
| requests: '>=2.19.0' | |
| requests: '>=2.32.2' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| sortedcontainers: '>=2.0.5' | ||
| psutil: '>=5.7.2' | ||
| cytoolz: '>=0.10.1' |
There was a problem hiding this comment.
The PR replaces urllib3: '>=1.24.3' with three new dependencies: sortedcontainers: '>=2.0.5', psutil: '>=5.7.2', and cytoolz: '>=0.10.1'. However, it appears that urllib3 is still required as it's present in the updated code at line 4455. Consider keeping urllib3 dependency alongside the new dependencies to maintain compatibility.
Code suggestion
Check the AI-generated fix before applying
| sortedcontainers: '>=2.0.5' | |
| psutil: '>=5.7.2' | |
| cytoolz: '>=0.10.1' | |
| urllib3: '>=1.24.3' | |
| sortedcontainers: '>=2.0.5' | |
| psutil: '>=5.7.2' | |
| cytoolz: '>=0.10.1' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| jinja2: '>=3.1.2' | ||
| python: '>=3.9' | ||
| blinker: '>=1.9' | ||
| itsdangerous: '>=2.2' |
There was a problem hiding this comment.
The dependency itsdangerous: '>=2.2' has been replaced with click: '>=8.1.3'. However, looking at the updated code context, both dependencies are needed for Flask. The itsdangerous dependency is still present at line 5007, but it appears this change might have been intended to add click rather than replace itsdangerous. Consider keeping both dependencies.
Code suggestion
Check the AI-generated fix before applying
| itsdangerous: '>=2.2' | |
| click: '>=8.1.3' | |
| itsdangerous: '>=2.2' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| proto-plus: '>=1.25.0,<2.0.0' | ||
| google-auth: '>=2.14.1,<3.0.0' | ||
| googleapis-common-protos: '>=1.56.2,<2.0.0' | ||
| protobuf: '>=3.19.5,<7.0.0,!=3.20.0,!=3.20.1,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5' |
There was a problem hiding this comment.
The protobuf version constraint has been updated to allow versions up to <7.0.0 in the osx-arm64 platform, while the osx-64 platform still uses <6.0.0.dev0. This inconsistency might cause compatibility issues when the same code runs on different platforms. Consider aligning the version constraints across platforms.
Code suggestion
Check the AI-generated fix before applying
| protobuf: '>=3.19.5,<7.0.0,!=3.20.0,!=3.20.1,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5' | |
| protobuf: '>=3.19.5,<6.0.0.dev0,!=3.20.0,!=3.20.1,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| protobuf: '>=3.20.2,<6.0.0dev,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5' | ||
| proto-plus: '>=1.22.3,<2.0.0dev' | ||
| tqdm: '>=4.7.4,<=5.0.0dev' | ||
| pyarrow: '>=3.0.0' |
There was a problem hiding this comment.
The diff is removing bigquery-magics: '>=0.1.0' dependency but it appears to still be used in the updated code (line 6692). Consider keeping this dependency to avoid potential runtime errors.
Code suggestion
Check the AI-generated fix before applying
@@ -6680,6 +6680,7 @@
+ python: '>=3.9'
+ pandas: '>=1.1.0'
+ protobuf: '>=3.20.2,<6.0.0dev,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5'
+ proto-plus: '>=1.22.3,<2.0.0dev'
+ tqdm: '>=4.7.4,<=5.0.0dev'
+ pyarrow: '>=3.0.0'
+ bigquery-magics: '>=0.1.0'
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| pandas: '>=0.21.1' | ||
| pyarrow: '>=0.15.0' | ||
| python: '>=3.9' | ||
| google-cloud-bigquery-storage-core: 2.27.0.* |
There was a problem hiding this comment.
The diff shows that pyarrow: '>=0.15.0' and python: '>=3.9' dependencies are being removed and replaced with google-cloud-bigquery-storage-core: 2.27.0.*. Looking at the updated code context, it appears that pyarrow and python dependencies are still present in the file (lines 6786-6787 and 6799-6800), but they might be missing from this specific platform section. Consider keeping these dependencies if they're required for this platform configuration.
Code suggestion
Check the AI-generated fix before applying
| google-cloud-bigquery-storage-core: 2.27.0.* | |
| google-cloud-bigquery-storage-core: 2.27.0.* | |
| pyarrow: '>=0.15.0' | |
| python: '>=3.9' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| anyio: '>=3.0,<5.0' | ||
| certifi: '' | ||
| h11: '>=0.13,<0.15' |
There was a problem hiding this comment.
The change replaces certifi: '' with h11: '>=0.13,<0.15' in the dependencies. Looking at the context, it appears that certifi is still needed as it's present in other platform configurations (like osx-64 at line 8115). Consider adding certifi back to maintain consistency across platform configurations.
Code suggestion
Check the AI-generated fix before applying
| h11: '>=0.13,<0.15' | |
| h11: '>=0.13,<0.15' | |
| certifi: '' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @@ -8816,11 +8770,11 @@ package: | |||
| manager: conda | |||
| platform: osx-arm64 | |||
There was a problem hiding this comment.
The PR adds two new dependencies (pygments >= 2.4.0 and jedi >= 0.16) to the ipython package configuration in the lock file, while removing the stack_data dependency. This might impact the functionality of ipython. Consider keeping the stack_data dependency if it's still required, as it appears to be present in other ipython configurations in the file.
Code suggestion
Check the AI-generated fix before applying
| platform: osx-arm64 | |
| pygments: '>=2.4.0' | |
| jedi: '>=0.16' | |
| stack_data: '' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| pkgutil-resolve-name: '>=1.3.10' | ||
| python: '>=3.9' | ||
| jsonschema-specifications: '>=2023.03.6' |
There was a problem hiding this comment.
The dependency change from python: '>=3.9' to jsonschema-specifications: '>=2023.03.6' at this line appears to be a mistake. Looking at the surrounding context, both dependencies are needed separately as they serve different purposes. The Python version requirement should not be replaced by the jsonschema-specifications dependency.
Code suggestion
Check the AI-generated fix before applying
| jsonschema-specifications: '>=2023.03.6' | |
| jsonschema-specifications: '>=2023.03.6' | |
| python: '>=3.9' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| rfc3986-validator: '>0.1.0' | ||
| uri-template: '' | ||
| jsonschema: '>=4.23.0,<4.23.1.0a0' |
There was a problem hiding this comment.
The change replaces uri-template: '' with jsonschema: '>=4.23.0,<4.23.1.0a0'. Looking at the surrounding context, it appears that uri-template is still needed as a dependency (it exists in other sections of the file). Consider adding back the uri-template dependency rather than replacing it.
Code suggestion
Check the AI-generated fix before applying
| jsonschema: '>=4.23.0,<4.23.1.0a0' | |
| jsonschema: '>=4.23.0,<4.23.1.0a0' | |
| uri-template: '' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| importlib-metadata: '' | ||
| nbclient: '>=0.2' | ||
| attrs: '' |
There was a problem hiding this comment.
The dependency attrs is being replaced with pyyaml at line 9555. However, looking at the updated code context, both dependencies appear to be needed as attrs is still present at line 9559. Consider adding pyyaml while keeping attrs instead of replacing it.
Code suggestion
Check the AI-generated fix before applying
| attrs: '' | |
| attrs: '' | |
| pyyaml: '' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| namex: '' | ||
| numpy: '' | ||
| ml_dtypes: '' |
There was a problem hiding this comment.
The dependency numpy has been replaced with ml_dtypes in this section of the lock file. However, looking at the updated code context, both dependencies appear to be needed in the keras package. Consider keeping both dependencies rather than replacing one with the other.
Code suggestion
Check the AI-generated fix before applying
| ml_dtypes: '' | |
| ml_dtypes: '' | |
| numpy: '' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| python: '>=3.9,<3.10.0a0' | ||
| python_abi: 3.9.* | ||
| url: https://conda.anaconda.org/conda-forge/osx-arm64/multiprocess-0.70.16-py39h06df861_1.conda | ||
| url: https://conda.anaconda.org/conda-forge/osx-arm64/multiprocess-0.70.15-py39h0f82c59_1.conda |
There was a problem hiding this comment.
The URL for multiprocess package has been downgraded from version 0.70.16 to 0.70.15. This appears to be inconsistent with the version declaration at line 15403 which specifies version 0.70.16. Consider updating the URL to match the declared version or update the version declaration to match the URL.
Code suggestion
Check the AI-generated fix before applying
| url: https://conda.anaconda.org/conda-forge/osx-arm64/multiprocess-0.70.15-py39h0f82c59_1.conda | |
| url: https://conda.anaconda.org/conda-forge/osx-arm64/multiprocess-0.70.16-py39h06df861_1.conda |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| python: '>=3.9' | ||
| importlib-metadata: '>=3.6' |
There was a problem hiding this comment.
The dependency importlib-metadata: '>=3.6' has been replaced with jupyterlab_pygments: ''. However, looking at the updated code context, importlib-metadata is still needed and appears on line 15808. This change might have been intended to add jupyterlab_pygments as a new dependency rather than replacing an existing one. Consider adding jupyterlab_pygments without removing importlib-metadata.
Code suggestion
Check the AI-generated fix before applying
| importlib-metadata: '>=3.6' | |
| jupyterlab_pygments: '' | |
| importlib-metadata: '>=3.6' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| pandas: '' | ||
| pillow: '>=1.1.6' | ||
| packaging: '' |
There was a problem hiding this comment.
The change replaces pillow: '>=1.1.6' with packaging: '' at line 15975. However, looking at the updated code context, both dependencies are needed in this section. Consider keeping both dependencies instead of replacing one with the other.
Code suggestion
Check the AI-generated fix before applying
| packaging: '' | |
| packaging: '' | |
| pillow: '>=1.1.6' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| httpx: '>=0.23.0,<1' | ||
| jiter: '>=0.4.0,<1' | ||
| distro: '>=1.7.0,<2' |
There was a problem hiding this comment.
The dependency jiter has been replaced with distro in this section of the lock file. However, looking at the updated code context, both dependencies are actually needed as they appear together in other sections (e.g., lines 16482 and 16507). This change might be unintentional and could cause dependency issues.
Code suggestion
Check the AI-generated fix before applying
| distro: '>=1.7.0,<2' | |
| distro: '>=1.7.0,<2' | |
| jiter: '>=0.4.0,<1' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| opentelemetry-semantic-conventions: 0.52b0 | ||
| packaging: '>=18.0' | ||
| python: '>=3.9' |
There was a problem hiding this comment.
The Python version requirement has been updated from >=3.7 to >=3.9. This change might break compatibility with Python 3.7 and 3.8 environments. Consider whether this is an intentional upgrade requirement or if backward compatibility should be maintained.
Code suggestion
Check the AI-generated fix before applying
| opentelemetry-semantic-conventions: 0.52b0 | |
| packaging: '>=18.0' | |
| python: '>=3.9' | |
| opentelemetry-semantic-conventions: 0.52b0 | |
| packaging: '>=18.0' | |
| python: '>=3.7' |
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| pyarrow: '>=4.0.0' | ||
| python: '>=3.9' | ||
| url: https://conda.anaconda.org/conda-forge/noarch/pyspark-3.5.4-pyhd8ed1ab_0.conda | ||
| url: https://conda.anaconda.org/conda-forge/noarch/pyspark-3.5.5-pyhd8ed1ab_0.conda |
There was a problem hiding this comment.
The URL for the pyspark package has been updated from version 3.5.4 to 3.5.5, but the dependencies section that was present in the original file has been removed. This could lead to missing dependency information when the package is installed or used.
Code suggestion
Check the AI-generated fix before applying
@@ -19700,2 +19700,4 @@
py4j: 0.10.9.7
+ pyarrow: '>=4.0.0'
+ python: '>=3.9'
url: https://conda.anaconda.org/conda-forge/noarch/pyspark-3.5.5-pyhd8ed1ab_0.conda
Code Review Run #66e219
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
What's the next steps for this? I'm in the process of building out our Flyte management infrastructure and would really love this feature. |
|
@machichima do you still want to land this? If so I can take a look and we can merge this. |
Yes please! Please let me know if anything needs to be improved |
5 participants
Tracking issue
#6237
Why are the changes needed?
Currently, configuring over multiple projects / domains can be painful as multiple commands need to be used for each setting.
What changes were proposed in this pull request?
A kubectl like config method is proposed, which enable providing multiple config simultaneously through
flytectl update --attrFile config.yml.config.ymlinclude configs from different subcommands specified bykindkeyword (see example below). Multiple config file is also supported here by usingflytectl update --attrFile config.yml --attrFile config1.yml.How was this patch tested?
Unit test
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
https://flyte--6292.org.readthedocs.build/en/6292/api/flytectl/gen/flytectl_update.html
Summary by Bito
This PR enhances flytectl's workflow execution by adding kubectl-like configuration support for multiple attribute files individually or via a directory. It modernizes file operations by replacing deprecated packages, refactors the configuration structure, and improves error handling. The changes include documentation updates reflecting these new capabilities and extensive updates to the monodocs-environment.lock.yaml file with dependency version changes.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5