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

Only require default plugin configuration for task types with multiple registered plugins#405

Merged
hamersaw merged 8 commits into
masterfrom
bug/fix-default-plugins
Mar 10, 2022
Merged

Only require default plugin configuration for task types with multiple registered plugins#405
hamersaw merged 8 commits into
masterfrom
bug/fix-default-plugins

Conversation

@hamersaw
Copy link
Copy Markdown
Member

TL;DR

There was a corner case where the aws_batch and k8s-array plugins could not be registered simultaneously. This fixes it.

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

With this change the registration of plugins for specific task types occurs such that:
(1) task types with only one registered plugin are automatically defaulted to that plugin
(2) task types with multiple registered plugins require the default-for-task-type configuration to define the default plugin

Default plugin information resident in the PluginEntry for each plugin is no longer used in determining the default plugin for a task type.

Tracking Issue

fixes flyteorg/flyte#2205

Follow-up issue

NA

… default plugin in config

Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: Daniel Rammer <daniel@union.ai>
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 25, 2022

Codecov Report

Merging #405 (bc0d66f) into master (163824c) will decrease coverage by 0.01%.
The diff coverage is 84.61%.

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

Comment thread pkg/controller/nodes/task/config/config.go
Comment thread pkg/controller/nodes/task/plugin_config.go Outdated
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw hamersaw dismissed a stale review via 61022cd March 1, 2022 16:57
EngHabu
EngHabu previously approved these changes Mar 1, 2022
Copy link
Copy Markdown
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

nits

Comment thread pkg/controller/nodes/task/plugin_config.go Outdated
hamersaw added 2 commits March 1, 2022 12:40
Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: Daniel Rammer <daniel@union.ai>
hamersaw added 2 commits March 2, 2022 11:27
Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: Daniel Rammer <daniel@union.ai>
@hamersaw hamersaw merged commit c1bf6d5 into master Mar 10, 2022
@hamersaw hamersaw deleted the bug/fix-default-plugins branch March 10, 2022 20:16
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
…e registered plugins (flyteorg#405)

* task types with only one registered plugin use it - otherwise declare default plugin in config

Signed-off-by: Daniel Rammer <daniel@union.ai>

* fixed test and lint issues

Signed-off-by: Daniel Rammer <daniel@union.ai>

* updated task-config flags

Signed-off-by: Daniel Rammer <daniel@union.ai>

* converted to named return values

Signed-off-by: Daniel Rammer <daniel@union.ai>

* failing to initialize plugins on nil TaskConfig

Signed-off-by: Daniel Rammer <daniel@union.ai>

* updated flyteplugins version

Signed-off-by: Daniel Rammer <daniel@union.ai>

* updated flyteplugins to merged version

Signed-off-by: Daniel Rammer <daniel@union.ai>
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.

[BUG] Enable k8s array plugin and aws batch plugin simultaneously

2 participants