Feat/#661 - Exposing creationTime and modificationTime #677

tharun208 · 2020-04-14T12:59:01Z

Summary

Exposed creationTime and modifiedTime from rest-api

Full changelog

Changed resource to expose creationTime and modificationTime
Updated tests

Issues resolved

Fix #661

* added creationTime and modificationTime in resource Feature kumahq#661

* updated api server tests Feature kumahq#661

* updated tests Feature kumahq#661

…jections * updated tests Feature kumahq#661

…hecks * updated tests Feature kumahq#661

* updated tests Feature kumahq#661

…mplates * updated tests Feature kumahq#661

…logs * updated tests Feature kumahq#661

…permissions * updated tests Feature kumahq#661

…routes * updated tests Feature kumahq#661

…traces * updated tests Feature kumahq#661

…planes * updated tests Feature kumahq#661

* fixed tests for printer Feature kumahq#661

* fixed remote store create and update function and updated tests Feature kumahq#661

* fixed apply tests Feature kumahq#661

Feature kumahq#661

jakubdyszkiewicz · 2020-04-15T13:45:00Z

pkg/plugins/resources/remote/store.go

+		Type:         string(res.GetType()),
+		Name:         opts.Name,
+		Mesh:         opts.Mesh,
+		CreationTime: opts.CreationTime,


This should not be set by the client. CreationTime is set on the server side.

then during creation or modification, I need to remove the createdAt and modifiedAt in kumactl apply right ?

jakubdyszkiewicz · 2020-04-15T13:45:08Z

pkg/plugins/resources/remote/store.go

+		Type:             string(res.GetType()),
+		Name:             res.GetMeta().GetName(),
+		Mesh:             res.GetMeta().GetMesh(),
+		ModificationTime: opts.ModificationTime,


This should not be set by the client. ModificationTime is set on the server side.

jakubdyszkiewicz · 2020-04-20T07:28:39Z

app/kumactl/cmd/apply/apply.go

@@ -222,9 +224,9 @@ func (m meta) GetMesh() string {
 }

 func (m meta) GetCreationTime() time.Time {
-	return time.Unix(0, 0) // the date doesn't matter since it is set on server side anyways
+	return m.CreationTime


Comment is still relevant. This should be set on the server-side.

…umactl

jakubdyszkiewicz · 2020-04-22T08:41:59Z

Hey, I tested this functionality and I found some issues that creation and modification time is always zero. After debugging it a bit I found the solution.

Index: pkg/plugins/resources/remote/unmarshalling.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- pkg/plugins/resources/remote/unmarshalling.go	(revision 93171c8dfd9c82eb6b2f1d26e9f36afcef3f4501)
+++ pkg/plugins/resources/remote/unmarshalling.go	(date 1587544615198)
@@ -43,10 +43,11 @@
 		return err
 	}
 	res.SetMeta(remoteMeta{
-		Name:    restResource.Meta.Name,
-		Mesh:    restResource.Meta.Mesh,
-		Version: "",
-		// todo(jakubdyszkiewicz) creation and modification time is not set because it's not exposed in API yet
+		Name:             restResource.Meta.Name,
+		Mesh:             restResource.Meta.Mesh,
+		Version:          "",
+		CreationTime:     restResource.Meta.CreationTime,
+		ModificationTime: restResource.Meta.ModificationTime,
 	})
 	return nil
 }
@@ -64,10 +65,11 @@
 			return err
 		}
 		r.SetMeta(&remoteMeta{
-			Name:    ri.Meta.Name,
-			Mesh:    ri.Meta.Mesh,
-			Version: "",
-			// todo(jakubdyszkiewicz) creation and modification time is not set because it's not exposed in API yet
+			Name:             ri.Meta.Name,
+			Mesh:             ri.Meta.Mesh,
+			Version:          "",
+			CreationTime:     ri.Meta.CreationTime,
+			ModificationTime: ri.Meta.ModificationTime,
 		})
 		_ = rs.AddItem(r)
 	}
Index: pkg/core/resources/manager/manager.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- pkg/core/resources/manager/manager.go	(revision 93171c8dfd9c82eb6b2f1d26e9f36afcef3f4501)
+++ pkg/core/resources/manager/manager.go	(date 1587544457296)
@@ -47,13 +47,13 @@
 	if err := resource.Validate(); err != nil {
 		return err
 	}
-	opts := store.NewCreateOptions(append(fs, store.CreatedAt(time.Now()))...)
+	opts := store.NewCreateOptions(fs...)
 	if resource.GetType() != mesh.MeshType {
 		if err := r.ensureMeshExists(ctx, opts.Mesh); err != nil {
 			return err
 		}
 	}
-	return r.Store.Create(ctx, resource, fs...)
+	return r.Store.Create(ctx, resource, append(fs, store.CreatedAt(time.Now()))...)
 }
 
 func (r *resourcesManager) ensureMeshExists(ctx context.Context, meshName string) error {

here is a patch for it.
Also please add tests to pkg/plugins/resources/remote/store_test.go to ensure that unmarshalling the times actually works. Thank you.

jakubdyszkiewicz

Also include CHANGELOG. Thanks.

jakubdyszkiewicz · 2020-04-22T08:42:24Z

app/kumactl/cmd/apply/apply.go

+	Name             string
+	Mesh             string
+	CreationTime     time.Time
+	ModificationTime time.Time


Is it needed though? I think we can revert this file to master

but not sure if I revert it to master, GetCreationTime() is time.Unix and it will be different for each machine and the test will fail in ci.

time.Unix(0, 0) indicates the constant time

jakubdyszkiewicz · 2020-04-22T08:43:10Z

app/kumactl/cmd/get/get_dataplanes_test.go

 	BeforeEach(func() {
+


Can you revert such files (I see a couple of them) with an empty line change to master?
It's easier to review and less potential conflicts

tharun208 · 2020-04-23T14:16:07Z

Hey, I tested this functionality and I found some issues that creation and modification time is always zero. After debugging it a bit I found the solution.

Index: pkg/plugins/resources/remote/unmarshalling.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- pkg/plugins/resources/remote/unmarshalling.go	(revision 93171c8dfd9c82eb6b2f1d26e9f36afcef3f4501)
+++ pkg/plugins/resources/remote/unmarshalling.go	(date 1587544615198)
@@ -43,10 +43,11 @@
 		return err
 	}
 	res.SetMeta(remoteMeta{
-		Name:    restResource.Meta.Name,
-		Mesh:    restResource.Meta.Mesh,
-		Version: "",
-		// todo(jakubdyszkiewicz) creation and modification time is not set because it's not exposed in API yet
+		Name:             restResource.Meta.Name,
+		Mesh:             restResource.Meta.Mesh,
+		Version:          "",
+		CreationTime:     restResource.Meta.CreationTime,
+		ModificationTime: restResource.Meta.ModificationTime,
 	})
 	return nil
 }
@@ -64,10 +65,11 @@
 			return err
 		}
 		r.SetMeta(&remoteMeta{
-			Name:    ri.Meta.Name,
-			Mesh:    ri.Meta.Mesh,
-			Version: "",
-			// todo(jakubdyszkiewicz) creation and modification time is not set because it's not exposed in API yet
+			Name:             ri.Meta.Name,
+			Mesh:             ri.Meta.Mesh,
+			Version:          "",
+			CreationTime:     ri.Meta.CreationTime,
+			ModificationTime: ri.Meta.ModificationTime,
 		})
 		_ = rs.AddItem(r)
 	}
Index: pkg/core/resources/manager/manager.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- pkg/core/resources/manager/manager.go	(revision 93171c8dfd9c82eb6b2f1d26e9f36afcef3f4501)
+++ pkg/core/resources/manager/manager.go	(date 1587544457296)
@@ -47,13 +47,13 @@
 	if err := resource.Validate(); err != nil {
 		return err
 	}
-	opts := store.NewCreateOptions(append(fs, store.CreatedAt(time.Now()))...)
+	opts := store.NewCreateOptions(fs...)
 	if resource.GetType() != mesh.MeshType {
 		if err := r.ensureMeshExists(ctx, opts.Mesh); err != nil {
 			return err
 		}
 	}
-	return r.Store.Create(ctx, resource, fs...)
+	return r.Store.Create(ctx, resource, append(fs, store.CreatedAt(time.Now()))...)
 }
 
 func (r *resourcesManager) ensureMeshExists(ctx context.Context, meshName string) error {

here is a patch for it.
Also please add tests to pkg/plugins/resources/remote/store_test.go to ensure that unmarshalling the times actually works. Thank you.

Hi Jakub, Can I update the existing tests with unmarshalling for unmarshalling the time? or add separate tests.

jakubdyszkiewicz · 2020-04-24T07:26:28Z

Hi Jakub, Can I update the existing tests with unmarshalling for unmarshalling the time? or add separate tests.

Update existing please

…time

jakubdyszkiewicz · 2020-04-27T10:21:08Z

thank you for the contribution

tharun208 added 18 commits April 14, 2020 16:56

feature exposed creationTime & modificationTime in kuma

665fd13

* added creationTime and modificationTime in resource Feature kumahq#661

feature exposed creationTime & modificationTime in kuma

5706587

* updated api server tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get dataplanes

dd2e5b1

* updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get fault in…

cc845a1

…jections * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get health c…

af336c9

…hecks * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get meshes

fbc8468

* updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get proxy Te…

7f55289

…mplates * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get traffic …

a3198ed

…logs * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get traffic …

e8a7445

…permissions * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get traffic …

6fe53f3

…routes * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma get traffic …

d8dc012

…traces * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma inspect data…

fd0ba3c

…planes * updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma

e104eb0

* fixed tests for printer Feature kumahq#661

feature exposed creationTime & modificationTime for kuma

6acce39

* fixed remote store create and update function and updated tests Feature kumahq#661

feature exposed creationTime & modificationTime for kuma

0d80de7

* fixed apply tests Feature kumahq#661

* update api-server resource tests with proper dateTime

080c42e

Feature kumahq#661

* removed unwanted omitempty tag for creationTime and modificationTime

1527053

Feature kumahq#661

* updated apply dry-run to return noop date-time

99117f7

Feature kumahq#661

tharun208 marked this pull request as ready for review April 14, 2020 18:41

tharun208 requested a review from a team April 14, 2020 18:41

tharun208 marked this pull request as draft April 14, 2020 18:43

tharun208 marked this pull request as ready for review April 15, 2020 07:18

tharun208 changed the title ~~Feat/#661~~ Feat/#661 - Exposing creationTime and modificationTime Apr 15, 2020

tharun208 added 2 commits April 15, 2020 14:54

Merge remote-tracking branch 'origin/master' into feat/kumahq#661

84162e1

Merge remote-tracking branch 'origin/master' into feat/kumahq#661

522f1e8

tharun208 marked this pull request as draft April 15, 2020 13:59

tharun208 added 2 commits April 15, 2020 20:29

* updated get single resource test data

f57aa4f

Feature kumahq#661

* added createdAt and modifiedAt params to upsert function in apply

e5ed83b

Feature kumahq#661

tharun208 force-pushed the feat/#661 branch from 2701594 to e5ed83b Compare April 15, 2020 15:05

tharun208 marked this pull request as ready for review April 15, 2020 15:18

tharun208 added 2 commits April 18, 2020 01:12

updated branch with upstream

0df63c9

fiixed failing tests

196d6ea

jakubdyszkiewicz reviewed Apr 20, 2020

View reviewed changes

tharun208 added 4 commits April 21, 2020 21:12

removed storing creation and modification time in client

8589237

show default creationTime and modificationTime for all resources in k…

dc0c940

…umactl

Merge remote-tracking branch 'origin/master' into feat/kumahq#661

a5b7ad4

show default creationTime and modificationTime in store

93171c8

tharun208 requested a review from jakubdyszkiewicz April 21, 2020 16:31

jakubdyszkiewicz requested changes Apr 22, 2020

View reviewed changes

tharun208 added 4 commits April 25, 2020 00:39

fix(kuma-ctl): reverted back creation and modification time to constant

c072066

fix(kuma-ctl): unmarshalling check for creationtime and modification …

ac3636c

…time

rebased with upstream

64a28e7

updated tests

87cdfdc

tharun208 requested a review from jakubdyszkiewicz April 24, 2020 19:16

jakubdyszkiewicz approved these changes Apr 27, 2020

View reviewed changes

jakubdyszkiewicz merged commit 7aa8726 into kumahq:master Apr 27, 2020

tharun208 deleted the feat/#661 branch April 27, 2020 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/#661 - Exposing creationTime and modificationTime #677

Feat/#661 - Exposing creationTime and modificationTime #677

tharun208 commented Apr 14, 2020

jakubdyszkiewicz Apr 15, 2020

tharun208 Apr 21, 2020

jakubdyszkiewicz Apr 21, 2020

tharun208 Apr 21, 2020 •

edited

jakubdyszkiewicz Apr 15, 2020

tharun208 Apr 21, 2020

jakubdyszkiewicz Apr 20, 2020

jakubdyszkiewicz commented Apr 22, 2020

jakubdyszkiewicz left a comment

jakubdyszkiewicz Apr 22, 2020

tharun208 Apr 23, 2020

jakubdyszkiewicz Apr 24, 2020

tharun208 Apr 24, 2020

jakubdyszkiewicz Apr 22, 2020

tharun208 commented Apr 23, 2020

jakubdyszkiewicz commented Apr 24, 2020

jakubdyszkiewicz commented Apr 27, 2020

Feat/#661 - Exposing creationTime and modificationTime #677

Feat/#661 - Exposing creationTime and modificationTime #677

Conversation

tharun208 commented Apr 14, 2020

Summary

Full changelog

Issues resolved

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tharun208 Apr 21, 2020 • edited

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

jakubdyszkiewicz commented Apr 22, 2020

jakubdyszkiewicz 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

tharun208 commented Apr 23, 2020

jakubdyszkiewicz commented Apr 24, 2020

jakubdyszkiewicz commented Apr 27, 2020

tharun208 Apr 21, 2020 •

edited