Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Conversation

Future-Outlier
Copy link
Member

TL;DR

As title.

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

Right now, the agent-service section doesn't need to add supportedTaskTypes anymore.
We can get it by "default-for-task-types" from tasks.
image
image

Tracking Issue

flyteorg/flytepropeller#612

@Future-Outlier Future-Outlier changed the title Add supportTaskTypes for agentservice without wrtie in config twice. Add supportTaskTypes for agentservice without write it in config twice. Sep 2, 2023
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: +1.29% 🎉

Comparison is base (08dd79b) 62.73% compared to head (f9cd2eb) 64.02%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   62.73%   64.02%   +1.29%     
==========================================
  Files         156      156              
  Lines       13172    10682    -2490     
==========================================
- Hits         8263     6839    -1424     
+ Misses       4284     3218    -1066     
  Partials      625      625              
Flag Coverage Δ
unittests ?

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

Files Changed Coverage Δ
go/tasks/plugins/webapi/agent/config.go 100.00% <ø> (ø)
go/tasks/plugins/webapi/agent/plugin.go 68.71% <60.00%> (+2.81%) ⬆️

... and 135 files with indirect coverage changes

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

pingsutw
pingsutw previously approved these changes Sep 13, 2023
Future Outlier added 3 commits September 21, 2023 06:40
… into add-supportedTaskTypes-for-agent-service-by-default-for-task-types
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@pingsutw pingsutw merged commit 2598c96 into flyteorg:master Sep 22, 2023
7 checks passed
eapolinario pushed a commit that referenced this pull request Sep 28, 2023
…e. (#398)

Signed-off-by: Future Outlier <[email protected]>
Co-authored-by: Future Outlier <[email protected]>
@honnix
Copy link
Member

honnix commented Oct 2, 2023

I'm not entirely sure this is a good direction to go. As I understood "default-for-task-types" is only needed when there is registration conflict, ie two plugins claiming to support the same type of task. This change to me feels like slightly abusing the semantics of "default-for-task-types". In our production setup for example, we have never needed to use "default-for-task-types", and now with this change we will be forced to use it. This is a breaking change to us.

Besides that, having this type of information scattered in different places of the configuration also doesn't seem to be ideal because it could be easy to miss something.

It would be great if this change can be re-evaluated and reverted. Thank you.

@pingsutw
Copy link
Member

pingsutw commented Oct 3, 2023

As I understood "default-for-task-types" is only needed when there is registration conflict, ie two plugins claiming to support the same type of task

@honnix sorry, This makes sense. We'll revert it shortly.

Future-Outlier added a commit to Future-Outlier/flyteplugins that referenced this pull request Oct 3, 2023
Future-Outlier added a commit to Future-Outlier/flyteplugins that referenced this pull request Oct 3, 2023
…fig twice. (flyteorg#398)"

This reverts commit 2598c96.

Signed-off-by: Future Outlier <[email protected]>
pingsutw pushed a commit that referenced this pull request Oct 4, 2023
…fig twice. (#398)" (#412)

This reverts commit 2598c96.

Signed-off-by: Future Outlier <[email protected]>
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.

3 participants