Fix #363: Creation of template with the same name #371

parauliya · 2020-11-23T14:22:57Z

Signed-off-by: parauliya [email protected]

Description

This PR will fix allow user to create a template with the same name after deleting it. Before this, it was not possible because of an issue also after deleting a template all the workflow which were created with that template before deleting it will be able to access the template since we template will be deleted softly.

Why is this needed

Fixes: #363

How Has This Been Tested?

Tested all the scenarios mentioned in the linked issue manually using vagrant setup.

How are existing users impacted? What migration steps/scripts do we need?

Have written a migration script for db.

Checklist:

I have:

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

gianarb · 2020-12-03T13:12:43Z

deploy/db/tinkerbell-init.sql

@@ -13,7 +13,7 @@ CREATE INDEX IF NOT EXISTS idxgin_type ON hardware USING GIN (data JSONB_PATH_OP

 CREATE TABLE IF NOT EXISTS template (
        id UUID UNIQUE NOT NULL
-        , name VARCHAR(200) UNIQUE NOT NULL
+        , name VARCHAR(200) NOT NULL


This is not used anymore. We use migrations now

https://github.com/tinkerbell/tink/tree/master/db/migration

You have to create a new migration file that ALTERS the current table

codecov · 2020-12-03T13:50:46Z

Codecov Report

Merging #371 (0f7e7de) into master (fe762e8) will increase coverage by 1.34%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
+ Coverage   24.74%   26.09%   +1.34%     
==========================================
  Files          14       14              
  Lines        1277     1280       +3     
==========================================
+ Hits          316      334      +18     
+ Misses        940      921      -19     
- Partials       21       25       +4

Impacted Files	Coverage Δ
grpc-server/workflow.go	`29.55% <81.81%> (+8.55%)`	⬆️
grpc-server/template.go	`36.53% <100.00%> (ø)`

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 fe762e8...0f7e7de. Read the comment docs.

gianarb

A few comments and tests have to be written for this PR

gianarb · 2020-12-04T12:17:12Z

db/template.go

@@ -14,7 +14,14 @@ import (

 // CreateTemplate creates a new workflow template
 func (d TinkDB) CreateTemplate(ctx context.Context, name string, data string, id uuid.UUID) error {
-	_, err := wflow.Parse([]byte(data))


Why are you doing this?! If the template is not valid we should not go further. Can you revert or explain why you moved this down there?

Yes you are right. I moved it down by mistake. Will fix it.

gianarb · 2020-12-04T12:17:54Z

db/db.go

@@ -37,6 +37,7 @@ type hardware interface {
 type template interface {
 	CreateTemplate(ctx context.Context, name string, data string, id uuid.UUID) error
 	GetTemplate(ctx context.Context, fields map[string]string) (string, string, string, error)
+	GetTemplateForWorkflow(ctx context.Context, fields map[string]string) (string, string, string, error)


I don't want to expose another API if not needed. Can you make ListTemplates smarter and remove this call?

I have made GetTemplate API smarter and removed this new API.

kqdeng · 2020-12-04T14:50:07Z

db/template.go

+	FROM template
+	WHERE
+		` + getCondition + `
+		created_at IS NOT NULL


when would created_at be null? why is this condition needed?

This code has been discarded.

Signed-off-by: parauliya <[email protected]>

… same name which is not deleted Signed-off-by: parauliya <[email protected]>

Signed-off-by: parauliya <[email protected]>

gianarb

It is good for me. Thank you for taking the time to add coverage.

I have to say, the more I look at the database package, and more I think we are doing something wrong in there. I would really like to write it in a more consistent way. But it is not part of this work for sure :D

Signed-off-by: parauliya <[email protected]>

ryli17 · 2020-12-09T19:08:23Z

db/migration/202012041103-remove-unique-constraints.go

+
+func Get202012041103() *migrate.Migration {
+	return &migrate.Migration{
+		Id: "202010221010-remove-unique-index",


The id is wrong. Please use the same timestamp in the function name.

ryli17 · 2020-12-09T19:56:36Z

db/template.go

+	fields := map[string]string{
+		"name": name,
+	}
+	_, _, _, err = d.GetTemplate(ctx, fields, false)


This query is not in the same transaction as the following insert DML, thus is not concurrent safe.

Introduced by #371 the migration has a wrong ID. Signed-off-by: Gianluca Arbezzano <[email protected]> Signed-off-by: Jonathan Wong <[email protected]>

## Description Some quick/shallow test for migration definition. ## Why is this needed #371 introduced a migration where none of the names matched :D, the Id & FuncName was fixed in #383 but that one missed the file name. Why make humans do things they're bad at when instead we can make a computer do something its good at? So this is that :D. ## How Has This Been Tested? Ran `go test` and it found the bad file name. I artificially changed an Id and the test caught the FuncName|Id mismatch. ## How are existing users impacted? What migration steps/scripts do we need? N/A ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [x] added unit or e2e tests - [ ] provided instructions on how to upgrade

parauliya added the kind/bug Categorizes issue or PR as related to a bug. label Nov 23, 2020

parauliya self-assigned this Nov 23, 2020

gianarb reviewed Dec 3, 2020

View reviewed changes

parauliya force-pushed the Fix_363 branch from 7e67724 to 060c42b Compare December 3, 2020 13:47

parauliya marked this pull request as ready for review December 3, 2020 14:19

parauliya changed the title ~~[WIP] : Fix #363: Creation of template with the same name~~ Fix #363: Creation of template with the same name Dec 3, 2020

parauliya requested review from kqdeng, gauravgahlot and Cbkhare December 3, 2020 14:20

parauliya force-pushed the Fix_363 branch from 0837b7f to 925b2d8 Compare December 4, 2020 06:46

parauliya requested a review from gianarb December 4, 2020 07:02

parauliya mentioned this pull request Dec 4, 2020

FIX #363: Added fix for template soft deletes #372

Closed

3 tasks

gianarb suggested changes Dec 4, 2020

View reviewed changes

gianarb removed request for Cbkhare, gauravgahlot and kqdeng December 4, 2020 12:21

kqdeng reviewed Dec 4, 2020

View reviewed changes

parauliya force-pushed the Fix_363 branch 2 times, most recently from 9825cb1 to c370c87 Compare December 7, 2020 05:09

parauliya added 3 commits December 7, 2020 10:40

Fix tinkerbell#363: Creation of template with the same name

e04e305

Signed-off-by: parauliya <[email protected]>

Added the check in CreateTemplate func for existing template with the…

ca6bf08

… same name which is not deleted Signed-off-by: parauliya <[email protected]>

Incorporate review comment and few optimisation

595ce83

Signed-off-by: parauliya <[email protected]>

parauliya force-pushed the Fix_363 branch from c370c87 to 595ce83 Compare December 7, 2020 05:11

Added UT for GetWorkflow function

dee0a3f

Signed-off-by: parauliya <[email protected]>

parauliya requested review from kqdeng and gianarb December 7, 2020 07:08

gianarb previously approved these changes Dec 7, 2020

View reviewed changes

gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Dec 7, 2020

Added UT for creating a template with same name after deleting one

0f7e7de

Signed-off-by: parauliya <[email protected]>

parauliya dismissed gianarb’s stale review via 0f7e7de December 7, 2020 13:36

parauliya force-pushed the Fix_363 branch from 1428c1e to 0f7e7de Compare December 7, 2020 13:36

kqdeng approved these changes Dec 7, 2020

View reviewed changes

mergify bot merged commit 1992c1c into tinkerbell:master Dec 7, 2020

ryli17 reviewed Dec 9, 2020

View reviewed changes

gianarb mentioned this pull request Dec 10, 2020

Fix migration id for template name key #383

Merged

jonawong pushed a commit that referenced this pull request Dec 11, 2020

Fix migration id for template name key

f4afc37

Introduced by #371 the migration has a wrong ID. Signed-off-by: Gianluca Arbezzano <[email protected]> Signed-off-by: Jonathan Wong <[email protected]>

mmlb mentioned this pull request Jan 23, 2021

Simple tests for db/migration #421

Merged

3 tasks

mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021

parauliya deleted the Fix_363 branch February 25, 2021 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #363: Creation of template with the same name #371

Fix #363: Creation of template with the same name #371

parauliya commented Nov 23, 2020 •

edited

Loading

gianarb Dec 3, 2020

parauliya Dec 4, 2020

codecov bot commented Dec 3, 2020 •

edited

Loading

gianarb left a comment

gianarb Dec 4, 2020

parauliya Dec 6, 2020

parauliya Dec 7, 2020

gianarb Dec 4, 2020

parauliya Dec 6, 2020

parauliya Dec 7, 2020

kqdeng Dec 4, 2020

parauliya Dec 6, 2020

gianarb left a comment

ryli17 Dec 9, 2020

ryli17 Dec 9, 2020

Fix #363: Creation of template with the same name #371

Fix #363: Creation of template with the same name #371

Conversation

parauliya commented Nov 23, 2020 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2020 • edited Loading

Codecov Report

gianarb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianarb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parauliya commented Nov 23, 2020 •

edited

Loading

codecov bot commented Dec 3, 2020 •

edited

Loading