Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

Fix the error message for the update cmd #411

Merged
merged 8 commits into from
Jul 6, 2023

Conversation

oliverhu
Copy link
Contributor

@oliverhu oliverhu commented Jul 3, 2023

Fix Flyte's #3806 [Housekeeping] Inconsistent treatment of project id param in flytectl

TL;DR

As project name is required rather than project id, check name instead.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

https://github.com/flyteorg/flyte/issues/

Updated behavior

❯ ./bin/flytectl update project -p flytesnack --id flytesnack3
Error: both project and id are passed
❯ ./bin/flytectl update project -p flytesnacks --name flytesnack3
Project flytesnacks updated
❯ ./bin/flytectl update project --id flytesnacks --name flytesnack3
Project flytesnacks updated```

cmd/update/project.go Outdated Show resolved Hide resolved
@oliverhu
Copy link
Contributor Author

oliverhu commented Jul 5, 2023

@katrogan updated, I think id is not expected to be updated, so added a check for that. Output:

❯ ./bin/flytectl update project -p flytesnack --id flytesnack3
Error: Project `id` can't be updated. Hint: did you mean `-p` instead of `--id` to specify the project?
❯ ./bin/flytectl update project -p flytesnack --name flytesnack3
Project flytesnack failed to update due to rpc error: code = NotFound desc = project [flytesnack] not found
Error: rpc error: code = NotFound desc = project [flytesnack] not found
❯ ./bin/flytectl update project -p flytesnacks --name flytesnack3
Project flytesnacks updated

Issue 3806 [Housekeeping] Inconsistent treatment of project id param in flytectl #3806

Signed-off-by: oliverhu <[email protected]>
Signed-off-by: oliverhu <[email protected]>
@katrogan
Copy link
Contributor

katrogan commented Jul 5, 2023

I think the issue is we also overwrite the project id to the global project flag: https://github.com/flyteorg/flytectl/blob/master/cmd/config/subcommand/project/project_config.go#L65 passed here: https://github.com/flyteorg/flytectl/blob/master/cmd/update/project.go#L87

Maybe we should only fallback to the global project flag when --id isn't specified

@oliverhu
Copy link
Contributor Author

oliverhu commented Jul 5, 2023

Yeah, I saw that. It is very confusing.. what about we change:
func (c *ConfigProject) GetProjectSpec(id string) (*admin.Project, error) { to func (c *ConfigProject) GetProjectSpec() (*admin.Project, error) {

So ProjectSpec will be a 1:1 mapping with the input.

Then we always get Project from config.GetConfig().Project?

@oliverhu oliverhu requested a review from katrogan July 5, 2023 23:30
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

Thank you for this!

cmd/config/subcommand/project/project_config.go Outdated Show resolved Hide resolved
cmd/config/subcommand/project/project_config.go Outdated Show resolved Hide resolved
cmd/update/project.go Outdated Show resolved Hide resolved
@oliverhu oliverhu requested a review from katrogan July 6, 2023 17:20
katrogan
katrogan previously approved these changes Jul 6, 2023
@katrogan
Copy link
Contributor

katrogan commented Jul 6, 2023

mind fixing up the lint & unit test failures?

Signed-off-by: oliverhu <[email protected]>
Signed-off-by: oliverhu <[email protected]>
@oliverhu oliverhu requested a review from katrogan July 6, 2023 19:11
@oliverhu
Copy link
Contributor Author

oliverhu commented Jul 6, 2023

Updated lint & unit tests, thanks for the patient reviews!

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #411 (ba65726) into master (0cb2bfa) will increase coverage by 0.98%.
The diff coverage is 80.00%.

❗ Current head ba65726 differs from pull request most recent head ebc2caa. Consider uploading reports for the commit ebc2caa to get more accurate results

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
+ Coverage   66.66%   67.64%   +0.98%     
==========================================
  Files         145      145              
  Lines        6285     5230    -1055     
==========================================
- Hits         4190     3538     -652     
+ Misses       1815     1411     -404     
- Partials      280      281       +1     
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
cmd/config/subcommand/project/project_config.go 73.80% <73.68%> (+0.47%) ⬆️
cmd/create/project.go 69.56% <100.00%> (+2.89%) ⬆️
cmd/update/project.go 66.66% <100.00%> (-16.67%) ⬇️
pkg/sandbox/start.go 61.01% <100.00%> (+7.29%) ⬆️

... and 130 files with indirect coverage changes

@katrogan katrogan merged commit 47d6c62 into flyteorg:master Jul 6, 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.

2 participants