Added update for named entity

[pmahindrakar-oss]

Signed-off-by: Prafulla Mahindrakar [email protected]

TL;DR

Following command updates the description on the workflow.

 flytectl update workflow -p flytectldemo -d development core.advanced.run_merge_sort.merge_sort --description "Mergesort workflow example"

Archiving workflow named entity would cause this to disapper from flyteconsole UI.

 flytectl update workflow -p flytectldemo -d development  core.advanced.run_merge_sort.merge_sort --archive

Activating workflow named entity would unarchive it.

 flytectl update workflow -p flytectldemo -d development  core.advanced.run_merge_sort.merge_sort --activate

Following command updates the description on the task.

 flytectl update  task -d development -p flytectldemo core.advanced.run_merge_sort.merge --description "Merge sort example"

Archiving task named entity would is not supported and would throw an error.

 flytectl update  task -d development -p flytectldemo core.advanced.run_merge_sort.merge --archive

Activating workflow named entity would be a noop as archiving is not possible.

 flytectl update  task -d development -p flytectldemo core.advanced.run_merge_sort.merge --activate

Following command updates the description on the launchplan.

 flytectl update launchplan -p flytectldemo -d development  core.advanced.run_merge_sort.merge_sort --description "Mergesort example"

Archiving launchplan named entity is not supported and would throw an error.

 flytectl update launchplan -p flytectldemo -d development  core.advanced.run_merge_sort.merge_sort --archive

Activating launchplan named entity would be a noop.

 flytectl update launchplan -p flytectldemo -d development  core.advanced.run_merge_sort.merge_sort --activate

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

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#919

Follow-up issue

[codecov]

Codecov Report

Merging #54 (a8b2875) into master (bf9a7ab) will increase coverage by 1.95%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   48.74%   50.69%   +1.95%     
==========================================
  Files          35       40       +5     
  Lines        1073     1150      +77     
==========================================
+ Hits          523      583      +60     
- Misses        485      500      +15     
- Partials       65       67       +2     

Flag	Coverage Δ
unittests	50.69% <80.00%> (+1.95%)	⬆️

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

Impacted Files	Coverage Δ
cmd/create/executionconfig_flags.go	40.00% <ø> (ø)
cmd/update/namedentityconfig_flags.go	29.41% <29.41%> (ø)
cmd/update/launch_plan.go	54.54% <54.54%> (ø)
cmd/update/named_entity.go	100.00% <100.00%> (ø)
cmd/update/project.go	100.00% <100.00%> (ø)
cmd/update/task.go	100.00% <100.00%> (ø)
cmd/update/update.go	100.00% <100.00%> (ø)
cmd/update/workflow.go	100.00% <100.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf9a7ab...a8b2875. Read the comment docs.

[katrogan]

have you considered raising an error for no-op commands (like activating a task, etc). I wonder if silently doing nothing is preferable as opposed to being more explicit

[pmahindrakar-oss]

Hi @katrogan i did consider raising an error for noop cases , but wont it be preferable to have this coming from admin at this line

if request.Metadata.State != admin.NamedEntityState_NAMED_ENTITY_ACTIVE &&
		request.ResourceType != core.ResourceType_WORKFLOW {
		return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
			"Only workflow name entities can have their state updated")
	}

https://github.com/flyteorg/flyteadmin/blob/2d81c1eec24cffb43346b56fc0017fd29db33a38/pkg/manager/impl/validation/named_entity_validator.go#L33

Since we already do it for archive state hence i thought why not have it for other state update checks aswell in admin to make it similar .
Let me know what you think

[kumare3]

@pmahindrakar-oss i dont know if I really prefer activate as the opposite of archive - this could be confusing, as activate for launch plan has an alternate meaning.
@katrogan wdyt?

[kumare3]

besides the one thing, I am +1 to merge it

[pmahindrakar-oss]

I will reword in documentation to make it clear.
The following statement might have caused this

Activating workflow named entity would unarchive it.

 flytectl update workflow -p flytectldemo -d development  core.advanced.run_merge_sort.merge_sort --activate

Will keep those separate

[katrogan]

@kumare3 not sure what a better name would be. in the proto def we also call it activate but agreed that word is potentially overloaded

[katrogan]

Hi @katrogan i did consider raising an error for noop cases , but wont it be preferable to have this coming from admin at this line

if request.Metadata.State != admin.NamedEntityState_NAMED_ENTITY_ACTIVE &&
		request.ResourceType != core.ResourceType_WORKFLOW {
		return errors.NewFlyteAdminErrorf(codes.InvalidArgument,
			"Only workflow name entities can have their state updated")
	}

https://github.com/flyteorg/flyteadmin/blob/2d81c1eec24cffb43346b56fc0017fd29db33a38/pkg/manager/impl/validation/named_entity_validator.go#L33

Since we already do it for archive state hence i thought why not have it for other state update checks aswell in admin to make it similar .
Let me know what you think

@pmahindrakar-oss I see, so even though it's technically a no-op we'll raise the error that admin returns?

[pmahindrakar-oss]

Yes thats right @katrogan . Instead of adding a new validation it would be ok to jus re-throw the error from admin and i have currently worded it to be noop in the doc since there is no feedback given but once we have the error thrown from admin then we can change this to say that this is an error case and the user should be able to see similar error like archiving launchplan.

[kumare3]

@katrogan / @pmahindrakar-oss - So eventually we probably want archive for all entities right? - LaunchPlan and Tasks. So there are 3 ways of solving this

1. Do not expose this functionality in flytectl
2. Expose the functionality but raise an error on client side
3. Expose the functionality and also make an admin call, but let admin return an error

IMO, 3 is the most long term clean solution. But we need to determine if there is a case in which entity archival does not make sense. Can you guys resolve this?

[katrogan]

#3 sounds fine

[pmahindrakar-oss]

Thanks for the review. Merging it now and we will take of no.3 using the following issue flyteorg/flyte#943