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

Flyte Execution tags #571

Merged
merged 30 commits into from
Aug 7, 2023
Merged

Flyte Execution tags #571

merged 30 commits into from
Aug 7, 2023

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Jun 1, 2023

TL;DR

Add two tables (admin_tags, execution_admin_tags).
execution_admin_tags is a join table containing admin_tags's ID and execution's ID.
admin_tags save all the tag names.

To list executions that match specific tags, we will join the execution, execution_admin_tags, and admin_tags tables.

  • execution_admin_tags
Name Type
project (foreign key) string
domain (foreign key string
execution name (foreign key) string
tag_id (foreign key) unit
  • admin_tags
Name Type
id (primary key) unit
tag name (unique index) string (unique)
created_at time.Time
updated_at time.Time
deleted_at time.Time

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

image

Tracking Issue

flyteorg/flyte#3320
Blocked by flyteorg/flyteidl#414

Follow-up issue

NA

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #571 (f88bf6d) into master (4713861) will increase coverage by 1.26%.
Report is 2 commits behind head on master.
The diff coverage is 8.62%.

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

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
+ Coverage   58.70%   59.97%   +1.26%     
==========================================
  Files         168      168              
  Lines       16307    13383    -2924     
==========================================
- Hits         9573     8026    -1547     
+ Misses       5884     4506    -1378     
- Partials      850      851       +1     
Flag Coverage Δ
unittests ?

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

Files Changed Coverage Δ
pkg/manager/impl/execution_manager.go 73.03% <ø> (+2.54%) ⬆️
pkg/manager/impl/util/filters.go 71.54% <ø> (+3.80%) ⬆️
pkg/repositories/config/migrations.go 2.97% <0.00%> (-0.27%) ⬇️
pkg/repositories/gormimpl/common.go 67.85% <ø> (+5.00%) ⬆️
pkg/repositories/gormimpl/execution_repo.go 67.36% <16.66%> (-1.11%) ⬇️
pkg/repositories/transformers/execution.go 83.87% <100.00%> (+2.82%) ⬆️

... and 149 files with indirect coverage changes

Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Looking at the RFC we said we were going to have separate keys and values for labels, whereas the implementation in this PR we use flat values as the tags, right? It doesn't invalidate the overall approach (of having a many-to-many relationship) though.

pkg/repositories/models/execution.go Outdated Show resolved Hide resolved
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Looking good, just a couple of comments.

pkg/repositories/config/migrations.go Outdated Show resolved Hide resolved
pkg/repositories/models/execution.go Outdated Show resolved Hide resolved
pkg/repositories/transformers/execution.go Show resolved Hide resolved
Signed-off-by: Kevin Su <[email protected]>
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

I'm wondering if we have to worry about storage requirements at this point, so left a comment related to this.

Also, not sure if the queries to list executions are correct, so a couple comments in that area as well.

pkg/repositories/transformers/execution.go Show resolved Hide resolved
pkg/repositories/models/execution.go Show resolved Hide resolved
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Just a few comments on how to handle the creation of tags (and potential duplicates).

tests/execution_test.go Show resolved Hide resolved
pkg/repositories/transformers/execution.go Show resolved Hide resolved
tests/execution_test.go Show resolved Hide resolved
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw merged commit 04bcb15 into master Aug 7, 2023
12 of 14 checks passed
@pingsutw pingsutw deleted the execution_tags branch August 7, 2023 21:23
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* test

Signed-off-by: Kevin Su <[email protected]>

* Execution tags

Signed-off-by: Kevin Su <[email protected]>

* Add tags filter

Signed-off-by: Kevin Su <[email protected]>

* Add execution tags table

Signed-off-by: Kevin Su <[email protected]>

* update tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>

* update migration

Signed-off-by: Kevin Su <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

* use gorm size

Signed-off-by: Kevin Su <[email protected]>

* Add tests

Signed-off-by: Kevin Su <[email protected]>

* bump idl

Signed-off-by: Kevin Su <[email protected]>

* bump idl

Signed-off-by: Kevin Su <[email protected]>

* bump idl

Signed-off-by: Kevin Su <[email protected]>

* address comment

Signed-off-by: Kevin Su <[email protected]>

* Set tag_name column as unique

Signed-off-by: Kevin Su <[email protected]>

* Add integration tests

Signed-off-by: Kevin Su <[email protected]>

* Update the tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* BeforeCreate

Signed-off-by: Kevin Su <[email protected]>

* Update migration ID

Signed-off-by: Kevin Su <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

* bump idl

Signed-off-by: Kevin Su <[email protected]>

* update migration id

Signed-off-by: Kevin Su <[email protected]>

---------

Signed-off-by: Kevin Su <[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