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

Fix_475: Correct timestamps in get by Id commands for template and workflow #476

Merged
merged 3 commits into from
Apr 21, 2021

Conversation

parauliya
Copy link
Contributor

Signed-off-by: parauliya aman@infracloud.io

Description

After this change the value of createdAt and updatedAt will be correct in the output of tink template/workflow get <id> command.

Why is this needed

Fixes: #475

How Has This Been Tested?

This has been tested manually throgh vagrant setup.

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

No Impact

Checklist:

I have:

  • added unit or e2e tests
  • provided instructions on how to upgrade

@parauliya parauliya requested review from thebsdbox and gianarb April 8, 2021 07:36
@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #476 (ad9d1a7) into master (5d58c4b) will increase coverage by 0.54%.
The diff coverage is 95.91%.

❗ Current head ad9d1a7 differs from pull request most recent head c5fbe14. Consider uploading reports for the commit c5fbe14 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   32.16%   32.70%   +0.54%     
==========================================
  Files          44       44              
  Lines        3112     3137      +25     
==========================================
+ Hits         1001     1026      +25     
  Misses       2019     2019              
  Partials       92       92              
Impacted Files Coverage Δ
db/db.go 52.63% <ø> (ø)
db/template.go 52.08% <90.47%> (+4.35%) ⬆️
db/workflow.go 30.63% <100.00%> (+1.54%) ⬆️
grpc-server/template.go 36.53% <100.00%> (ø)
grpc-server/workflow.go 41.11% <100.00%> (+0.60%) ⬆️

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 5d58c4b...c5fbe14. Read the comment docs.

@parauliya parauliya self-assigned this Apr 8, 2021
@parauliya parauliya added the kind/bug Categorizes issue or PR as related to a bug. label Apr 8, 2021
@parauliya parauliya marked this pull request as ready for review April 8, 2021 10:55
@gauravgahlot gauravgahlot added the do-not-merge Signal to Mergify to block merging of the PR. label Apr 8, 2021
@@ -25,5 +26,5 @@ type DB struct {
InsertIntoWorkflowEventTableFunc func(ctx context.Context, wfEvent *pb.WorkflowActionStatus, time time.Time) error
// template
TemplateDB map[string]interface{}
GetTemplateFunc func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error)
GetTemplateFunc func(ctx context.Context, fields map[string]string, deleted bool) (*tb.WorkflowTemplate, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Replacing, set of strings with this struct tb.WorkflowTemplate is really good.

@gauravgahlot gauravgahlot removed the do-not-merge Signal to Mergify to block merging of the PR. label Apr 16, 2021
@@ -7,7 +7,7 @@ import (

"github.com/stretchr/testify/assert"
"github.com/tinkerbell/tink/db/mock"
pb "github.com/tinkerbell/tink/protos/template"
tb "github.com/tinkerbell/tink/protos/template"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't rename the package referer, some changes in the file would go away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought that earlier, but since we are using pb as a package referrer for workflow and tb for package referrer for template at many other places so if I use pb for template package referrer it can cause confusions around the code readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have replaced tb with pb since there was only template package in that file.

@parauliya parauliya requested a review from gauravgahlot April 19, 2021 10:40
parauliya added 3 commits April 21, 2021 12:26
Signed-off-by: parauliya <aman@infracloud.io>
Signed-off-by: parauliya <aman@infracloud.io>
Signed-off-by: parauliya <aman@infracloud.io>
Copy link
Contributor

@gauravgahlot gauravgahlot left a comment

Choose a reason for hiding this comment

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

LTGM

@thebsdbox thebsdbox merged commit 89f7a80 into tinkerbell:master Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get command in tink-cli provides incorrect timestamp during get by Id
4 participants