Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed: duplicate key value violates unique constraint while creating multiple workflows #395

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Fixed: duplicate key value violates unique constraint while creating multiple workflows #395

merged 1 commit into from
Dec 18, 2020

Conversation

gauravgahlot
Copy link
Contributor

@gauravgahlot gauravgahlot commented Dec 16, 2020 •
edited
Loading

Why is this needed

Fixes: #379

How Has This Been Tested?

  1. git clone https://github.com/kinvolk/terraform-provider-tinkerbell && cd terraform-provider-tinkerbell
  2. build new images with changes in place and update docker-compose file
  3. docker-compose -f test/docker-compose.yml up -d
  4. TF_ACC=1 TINKERBELL_GRPC_AUTHORITY=127.0.0.1:42113 TINKERBELL_CERT_URL=http://127.0.0.1:42114/cert go test -v -run=^TestAccWorkflow_.*$ -count 100 ./tinkerbell/

@gauravgahlot gauravgahlot self-assigned this Dec 16, 2020
@gauravgahlot gauravgahlot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 16, 2020
@codecov
Copy link

codecov bot commented Dec 16, 2020 •
edited
Loading

Codecov Report

Merging #395 (f3a4074) into master (30bc3f9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #395   +/-   ##
=======================================
  Coverage   20.82%   20.82%           
=======================================
  Files          24       24           
  Lines        2170     2170           
=======================================
  Hits          452      452           
  Misses       1678     1678           
  Partials       40       40

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 617d56a...f3a4074. Read the comment docs.

gianarb
gianarb suggested changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

@gauravgahlot I think you have to create a separate migration :) so if we have people running the previous version they will be able to move from the master to this one

invidian
invidian reviewed Dec 16, 2020
View reviewed changes
Copy link
Contributor

@invidian invidian left a comment

Choose a reason for hiding this comment

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

I didn't get time to test it yet, but from what I've seen, it should fix the issue. Scheme also looks sensible, though some explanation in commit message would be nice, so one don't have to figure it out on their own.

@gianarb
Copy link
Contributor

gianarb commented Dec 16, 2020

Reading the comment @invidian said I think it will be nice to actually have that documentation as a comment in the migration function itself! ❤️

@mmlb
Copy link
Contributor

mmlb commented Dec 16, 2020

Reading the comment @invidian said I think it will be nice to actually have that documentation as a comment in the migration function itself! heart

This is probably a good idea in general for all migrations :D

@gauravgahlot gauravgahlot force-pushed the fix-379 branch 2 times, most recently from 7423afc to 86f3145 Compare December 18, 2020 11:19
@gauravgahlot
change primary key from 'created_at' to 'id'
f3a4074 
We can have multiple events generated at a given time, therefore the value of
'created_at' field will be same for each event. This violates the unique constraint
on "events_pkey", when these events are add to the events table.

The migration changes the primary key on events table from 'created_at' to `id`,
which will always be unique for each event generated at any point in time.

Signed-off-by: Gaurav Gahlot <[email protected]>
invidian
invidian approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM

@gauravgahlot gauravgahlot added the ready-to-merge Signal to Mergify to merge the PR. label Dec 18, 2020
@gauravgahlot gauravgahlot requested review from gianarb and removed request for gianarb December 18, 2020 12:42
@mergify mergify bot merged commit a04ccb4 into tinkerbell:master Dec 18, 2020
@invidian invidian mentioned this pull request Dec 21, 2020
Closed
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@invidian invidian invidian approved these changes

@gianarb gianarb gianarb approved these changes

Assignees

@gauravgahlot gauravgahlot

Labels
priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

duplicate key value violates unique constraint "events_pkey" error while creating multiple workflows
4 participants
@gauravgahlot @gianarb @mmlb @invidian