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.

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

Add timestamp fields to database tables #678

Merged
merged 8 commits into from
Jun 26, 2019

Conversation

afiune
Copy link

@afiune afiune commented Jun 25, 2019

🔩 Description

We are adding timestamps to all our tables inside the applications-service.

👍 Definition of Done

Every table in the database has created_at and updated_at timestamps.

Additionally, we will start storing the timestamp of when habitat submitted
the event message. (new column in service table last_event_occurred_at)

👟 Demo Script / Repro Steps

Build and start the applications-service and run the integration tests,
you can also login to the database and list the data.

⛓️ Related Resources

https://chefio.atlassian.net/browse/A2-892

✅ Checklist

  • Necessary tests added/updated?
  • Necessary docs added/updated?
  • Code actually executed?
  • Vetting performed (unit tests, lint, etc.)?

@afiune afiune requested review from kmacgugan and danielsdeleo June 25, 2019 21:11
@afiune afiune self-assigned this Jun 25, 2019
@afiune afiune added the eas label Jun 25, 2019
var sg serviceGroup
err := db.SelectOne(&sg, "SELECT * FROM service_group WHERE id = $1", id)
var sgName string
err := db.SelectOne(&sgName, "SELECT name FROM service_group WHERE id = $1", id)
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@danielsdeleo danielsdeleo left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Salim Afiune added 5 commits June 26, 2019 07:32
@kmacgugan
Copy link

Can we add a test that ensures created_at timestamp doesn't get updated on update? Or just manually test that to make sure.

@afiune
Copy link
Author

afiune commented Jun 26, 2019

@kmacgugan already tested it since I added the function, triggers, and procedures. I would love if you could also test it to give your +1

I could also add a test but I am trying to slice my PRs so that they don't get too big.

@afiune
Copy link
Author

afiune commented Jun 26, 2019

@kmacgugan BTW Welcome back!!!

tenor-221985525

Copy link
Contributor

@danielsdeleo danielsdeleo left a comment

Choose a reason for hiding this comment

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

One of the tests could be strengthened. The rest LGTM 👍

assert.Nil(t, svc)
}

func TestStorageGetServiceFromUniqueFieldsEmptyParameters(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't seem to be testing much since the storage is empty. Maybe you could insert the hab event from the next test and then check that the following all return false:

suite.StorageClient.GetServiceFromUniqueFields("", "")
suite.StorageClient.GetServiceFromUniqueFields("postgres", "")
suite.StorageClient.GetServiceFromUniqueFields("", "1q2w3e4r")

Copy link
Author

Choose a reason for hiding this comment

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

Will do

Copy link
Author

Choose a reason for hiding this comment

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

Though, now that I am doing this modification, it doesn't matter if we have something in the database or not since we have verifications that check if any of the parameters is empty, we just return immediately without running a query. Should I still modify it?

if assert.True(t, exist) {
updatedOccurredAtTimestamp = svc.LastEventOccurredAt

// thea substraction from the initial and the updated timestamp should be greater than a second
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Salim Afiune added 2 commits June 26, 2019 21:33
Signed-off-by: Salim Afiune <[email protected]>
Signed-off-by: Salim Afiune <[email protected]>
Copy link

@kmacgugan kmacgugan left a comment

Choose a reason for hiding this comment

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

tenor-183781378

Copy link
Contributor

@danielsdeleo danielsdeleo left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants