Template revision #438

gauravgahlot · 2021-02-17T11:40:05Z

Description

The PR introduces template revisions. The template.Data field will not contain the actual template but a reference to a particular revision. Revisions are immutable, when you update a template you create a new revision.

Why is this needed

Fixes: #413

How Has This Been Tested?

Unit Tests
Manual Testing

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2021-03-01T08:51:10Z

Codecov Report

Merging #438 (0374b28) into master (1b178da) will increase coverage by 2.10%.
The diff coverage is 72.10%.

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

@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
+ Coverage   35.82%   37.92%   +2.10%     
==========================================
  Files          47       48       +1     
  Lines        2892     3032     +140     
==========================================
+ Hits         1036     1150     +114     
- Misses       1763     1775      +12     
- Partials       93      107      +14

Impacted Files	Coverage Δ
db/db.go	`45.45% <ø> (ø)`
db/migration/migration.go	`100.00% <ø> (ø)`
grpc-server/template.go	`37.76% <43.90%> (+1.22%)`	⬆️
db/template.go	`63.26% <69.38%> (+15.53%)`	⬆️
...12041103-template-with-same-name-are-acceptable.go	`100.00% <100.00%> (ø)`
...91055-add-partial-unique-constraint-on-template.go	`100.00% <100.00%> (ø)`
db/migration/202102111035-template-revisions.go	`100.00% <100.00%> (ø)`
db/workflow.go	`29.23% <100.00%> (+0.14%)`	⬆️
grpc-server/workflow.go	`40.10% <100.00%> (+0.32%)`	⬆️
... and 1 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 1b178da...c6602b4. Read the comment docs.

gianarb · 2021-03-03T13:19:19Z

db/db.go

@@ -42,6 +42,7 @@ type template interface {
 	DeleteTemplate(ctx context.Context, name string) error
 	ListTemplates(in string, fn func(id, n string, in, del *timestamp.Timestamp) error) error
 	UpdateTemplate(ctx context.Context, name string, data string, id uuid.UUID) error
+	ListTemplateRevisions(id string, fn func(id string, revision int, tCr *timestamp.Timestamp) error) error


Let's try to write something a bit more readable:

ListRevisionsByTemplateID(templateID string, fn func(revisionID string, revision int, tCr *timestamp.Timestamp) error) error

I am not sure what revisiononID string and revision int are

gianarb · 2021-03-03T14:31:21Z

db/db.go

@@ -81,6 +82,19 @@ func (t *TinkDB) Migrate() (int, error) {
 	return migrate.Exec(t.instance, "postgres", migration.GetMigrations(), migrate.Up)
 }

+// WithMigrations applies only the given migrations.
+// It is required to unit test migrations.
+func (t *TinkDB) WithMigrations(migrations []func() *migrate.Migration) error {


I don't think we need this function for mainly one reason. This is a new API that should not be used outside of tests. So it is in the wrong place, it should be exposed via TinkDB because it allows developers to use it outside of tests, and we do not want that.

Another reason is that it does not add much value or simplification you use it this way:

err := tinkDB.WithMigrations([]func() *migrate.Migration{ migration.Get202009171251, migration.Get202010071530, migration.Get202010221010, migration.Get202012041103, migration.Get202012091055, migration.Get2020121691335, })

But you can get to the same point writing:

_, err := migrate.Exec(db, "postgres", migrate.MemoryMigrationSource{ Migrations: []*migrate.Migration{ migration.Get202009171251(), migration.Get202010071530(), migration.Get202010221010(), migration.Get202012041103(), migration.Get202012091055(), migration.Get2020121691335(), }, }, migrate.Up)

Almost same line of code. If you are wondering where db is coming from:

db, tinkDB, cl := NewPostgresDatabaseClient(t, ctx, NewPostgresDatabaseRequest{})

gianarb · 2021-03-03T14:32:56Z

db/template_test.go

+		id := uuid.New().String()
+		w.ID = id
+		w.Name = fmt.Sprintf("id_%d", rand.Int())
+		err := createTemplateFromWorkflowType(ctx, tinkDB, w)


This test fails because the function CreateTemplate already relies on revision. Even if you didn't apply the migration yet, the Go code assumes revision to be in place.

In order to fix that you have to add a template not via code but with a SQL query.

gianarb · 2021-03-08T09:54:20Z

db/template_test.go

+	}
+}
+
+func createTemplatesFromSQL(ctx context.Context, db *sql.DB) error {


Can you add a comment? I am sure the next person or even myself in a couple of days will not remember why we have this function and the name is so generic that it won't help

gianarb · 2021-03-09T09:17:52Z

@gauravgahlot got from Dan and EM two databases containing different templates. We will use them as data sources for testing the migration against more data ❤️

gauravgahlot · 2021-03-11T08:11:40Z

@gauravgahlot got from Dan and EM two databases containing different templates. We will use them as data sources for testing the migration against more data ❤️

Tested with the databases and it's working as expected. 🚀

gianarb · 2021-03-11T09:00:56Z

When a workflow starts we have to keep track of the revision as well. To be able to figure out what it actually ran even after a couple of days, when it is likely possible that the template changed to a different revision.

In preparation for exposing the actual revisions do you mind creating two GRPC requests as part of the TemplateService:

ListRevisionsByTemplateID: This request takes a template ID and returns all the revision we have
GetRevision: this request takes templateID and a revisionID and it returns a single revision

Please write tests for this as well and document the new API as part of the proto file

Fixes: tinkerbell#413 The migration introduces template revisions. A template can have multiple revisions, which are stored in the 'template_revisions' table. The migration also transforms the existing templates to the new template-revisions structure. Therefore ensuring that end users do not face any issues with the introduction of revisions. Signed-off-by: Gaurav Gahlot <[email protected]>

Signed-off-by: Gaurav Gahlot <[email protected]>

As of today, the db.GetTemplate() method returns (string, string, string). The callers need to guess or look at the implementation code to figure out what is being returned. Signed-off-by: Gaurav Gahlot <[email protected]>

Signed-off-by: Gaurav Gahlot <[email protected]>

tstromberg · 2021-06-22T15:34:55Z

Is this PR ready to be re-reviewed? I noticed there were still some unresolved comments.

gauravgahlot · 2021-06-22T18:25:25Z

Is this PR ready to be re-reviewed? I noticed there were still some unresolved comments.

The PR aimed to solve the issue with tink events. Now that we don't have events, we don't really need this PR (unless we are planning to reintroduce events). The issue itself can be closed as well. However, it would be great if @thebsdbox and @gianarb can confirm.

Raj-Dharwadkar · 2021-06-29T15:38:44Z

@gianarb Please can you confirm ?

gianarb · 2021-06-29T15:41:09Z

I can confirm. Let's close it for now. Thanks a lot for your work I am sure if will be reused in the future

gauravgahlot self-assigned this Feb 17, 2021

gauravgahlot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 17, 2021

gauravgahlot added the do-not-merge Signal to Mergify to block merging of the PR. label Feb 17, 2021

gauravgahlot force-pushed the template-revision branch 2 times, most recently from ea407bd to ead2309 Compare February 23, 2021 09:52

gauravgahlot marked this pull request as ready for review February 25, 2021 09:53

gauravgahlot requested a review from gianarb February 25, 2021 09:54

gauravgahlot force-pushed the template-revision branch 2 times, most recently from c105880 to 117d200 Compare March 1, 2021 08:47

gauravgahlot force-pushed the template-revision branch 2 times, most recently from 5d0640c to e031ce4 Compare March 1, 2021 12:07

gianarb suggested changes Mar 3, 2021

View reviewed changes

gauravgahlot force-pushed the template-revision branch 2 times, most recently from bb5d9cf to e9eee26 Compare March 8, 2021 09:40

gauravgahlot removed the do-not-merge Signal to Mergify to block merging of the PR. label Mar 8, 2021

gauravgahlot requested a review from gianarb March 8, 2021 09:43

gianarb reviewed Mar 8, 2021

View reviewed changes

gianarb added the do-not-merge Signal to Mergify to block merging of the PR. label Mar 9, 2021

gianarb mentioned this pull request Mar 10, 2021

Clarification: Rename template into workflows and revisit what a workflow is today #452

Closed

gauravgahlot force-pushed the template-revision branch from e9eee26 to 44f2d71 Compare March 11, 2021 08:10

gauravgahlot requested a review from gianarb March 11, 2021 08:11

gauravgahlot removed the do-not-merge Signal to Mergify to block merging of the PR. label Mar 11, 2021

gauravgahlot force-pushed the template-revision branch 2 times, most recently from d1b7c1f to 9bbf4ef Compare March 12, 2021 04:34

gauravgahlot force-pushed the template-revision branch from 9bbf4ef to 74f3e07 Compare March 15, 2021 09:43

gauravgahlot added 8 commits March 16, 2021 13:07

test template-revision migration

7e2370b

Signed-off-by: Gaurav Gahlot <[email protected]>

template revision for operations

a78c0b4

Signed-off-by: Gaurav Gahlot <[email protected]>

unit tests

4c6097b

Signed-off-by: Gaurav Gahlot <[email protected]>

review fixes

ee02f8b

Signed-off-by: Gaurav Gahlot <[email protected]>

check if constraints exist

448a3c9

Signed-off-by: Gaurav Gahlot <[email protected]>

added RPCs to TemplateService

d09266a

Signed-off-by: Gaurav Gahlot <[email protected]>

service implementation and unit-test

95afc12

Signed-off-by: Gaurav Gahlot <[email protected]>

gauravgahlot force-pushed the template-revision branch from d6ef61c to 95afc12 Compare March 16, 2021 07:38

gauravgahlot added 2 commits March 17, 2021 15:31

db.GetTemplate() now returns db.Template

c0c2b2a

As of today, the db.GetTemplate() method returns (string, string, string). The callers need to guess or look at the implementation code to figure out what is being returned. Signed-off-by: Gaurav Gahlot <[email protected]>

workflow should have revision detail

c6602b4

Signed-off-by: Gaurav Gahlot <[email protected]>

gianarb mentioned this pull request Mar 17, 2021

0020: Proposal for Template Versioning tinkerbell/proposals#31

Merged

gianarb closed this Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template revision #438

Template revision #438

gauravgahlot commented Feb 17, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

gianarb Mar 3, 2021

gianarb Mar 3, 2021

gianarb Mar 3, 2021

gianarb Mar 8, 2021

gianarb commented Mar 9, 2021

gauravgahlot commented Mar 11, 2021

gianarb commented Mar 11, 2021 •

edited

Loading

tstromberg commented Jun 22, 2021

gauravgahlot commented Jun 22, 2021

Raj-Dharwadkar commented Jun 29, 2021

gianarb commented Jun 29, 2021

Template revision #438

Template revision #438

Conversation

gauravgahlot commented Feb 17, 2021 • edited Loading

Description

Why is this needed

How Has This Been Tested?

Checklist:

codecov bot commented Mar 1, 2021 • edited Loading

Codecov Report

gianarb Mar 3, 2021

Choose a reason for hiding this comment

gianarb Mar 3, 2021

Choose a reason for hiding this comment

gianarb Mar 3, 2021

Choose a reason for hiding this comment

gianarb Mar 8, 2021

Choose a reason for hiding this comment

gianarb commented Mar 9, 2021

gauravgahlot commented Mar 11, 2021

gianarb commented Mar 11, 2021 • edited Loading

tstromberg commented Jun 22, 2021

gauravgahlot commented Jun 22, 2021

Raj-Dharwadkar commented Jun 29, 2021

gianarb commented Jun 29, 2021

gauravgahlot commented Feb 17, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

gianarb commented Mar 11, 2021 •

edited

Loading