Set labels in workflow metadata #129
Conversation
catalinii
requested review from
anandswaminathan,
EngHabu,
katrogan,
kumare3,
matthewphsmith and
wild-endeavor
as code owners
catalinii
force-pushed
the
IC-269
branch
3 times, most recently
from
7db3014 to
27a5926
Compare
catalinii
force-pushed
the
IC-269
branch
3 times, most recently
from
bec8d05 to
f915978
Compare
There was a problem hiding this comment.
looks good! mind adding to the existing unit test: https://github.com/lyft/flyteadmin/blob/master/pkg/manager/impl/execution_manager_test.go#L199 to make sure we pass the project labels as expected?
|
to fix lint can you run |
go.mod
Outdated
| github.com/lyft/flyteidl v0.18.6 | ||
| github.com/lyft/flytepropeller v0.3.17 | ||
| github.com/lyft/flyteidl v0.18.7 | ||
| github.com/lyft/flytepropeller v0.4.2-0.20201001203502-c9d4b2a9b8dd |
There was a problem hiding this comment.
let's wait until the propeller changes are in (with an official release or tag) before merging this
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
==========================================
- Coverage 61.60% 61.59% -0.02%
==========================================
Files 105 105
Lines 7405 7421 +16
==========================================
+ Hits 4562 4571 +9
- Misses 2264 2268 +4
- Partials 579 582 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
catalinii
force-pushed
the
IC-269
branch
3 times, most recently
from
0c0b505 to
c8ee280
Compare
| labels := admin.Labels{ | ||
| Values: map[string]string{ | ||
| "label3": "3", | ||
| "label2": "1", // common label, will be dropped |
There was a problem hiding this comment.
do we want project labels to override individual labels?
There was a problem hiding this comment.
They do not override, see the code: https://github.com/lyft/flyteadmin/pull/129/files#diff-fc047e54b9dd82ca7c89ac9b32cb07b3R1375
There was a problem hiding this comment.
ah thank you for clarifying!
|
thank you looks good to me, just one comment and then +1 |
|
/merge |
flyteorg/flyte#521