Close 129/add optional version cleanup #196
Conversation
| @@ -137,6 +137,9 @@ container rename <old-name> <new-name> | |||
| container delete <name> | |||
| delete the given container | |||
|
|
|||
| container ttl <name> <ttl> | |||
There was a problem hiding this comment.
Maybe container set <container> <option> <value> and then there will be no need to create additional command/etc each similar case?
Or even it could be covered with container set <container> --ttl-hours 1234 --another-option 213123 - so to be typed and just add optional fields to protobuf?
This is just a different approach but looks promising for further similar changes since will allow to preserve proto and CLI interfaces. And another question is how to unset TTL i.e. remove it from container?
In addition probably --ttl could be added to container create?
| message SetContainerVersionsTTLRequest { | ||
| string namespace = 1; | ||
| string name = 2; | ||
| uint64 ttl_hours = 3; |
There was a problem hiding this comment.
Do you really expect 2^64 hours to be passed?
For instance 100 years = 876 000 ....
And comments above about the interface if we do really need to add a handler particularly for ttl
| @@ -60,6 +61,11 @@ func (m *protoClientMock) DeleteContainer(ctx context.Context, in *v1proto.Delet | |||
| return &v1proto.DeleteContainerResponse{}, args.Error(0) | |||
| } | |||
|
|
|||
| func (m *protoClientMock) SetContainerVersionsTTL(ctx context.Context, in *v1proto.SetContainerVersionsTTLRequest, opts ...grpc.CallOption) (*v1proto.SetContainerVersionsTTLResponse, error) { | |||
| args := m.Called(in.GetNamespace(), in.GetName(), time.Duration(in.GetTtlHours())*time.Hour) | |||
There was a problem hiding this comment.
There's no actual need to cast into time.Duration explicitly since any multiplication to time.Duration will be the same type.
Higher level question if we do really need to set conversion fringe here in CLI util or it could be right at service part on manager?
comment @ manager/presenter/grpc/proto/v1/manager.proto#59 is about that.
| return errors.Wrap(err, "error setting container versions TTL") | ||
| } | ||
|
|
||
| fmt.Printf("container `%s` versions TTL set to %s\n", containerName, ttl) |
There was a problem hiding this comment.
just about the form: "TTL for versions" but "version TTL"
:-)
| @@ -219,15 +224,16 @@ func main() { | |||
| r := router.New(ctx) | |||
|
|
|||
| r.Register(namespaceCreate.FullCommand(), cliSvc.CreateNamespace(*namespaceCreateName)) | |||
| r.Register(namespaceRename.FullCommand(), cliSvc.RenameNamespace(*containerRenameOldName, *containerRenameNewName)) | |||
| r.Register(namespaceRename.FullCommand(), cliSvc.RenameNamespace(*namespaceRenameOldName, *namespaceRenameNewName)) | |||
There was a problem hiding this comment.
Looks like a fix, would you mind to move it to another PR such as deleteObject...?
| "namespace": namespace, | ||
| "container": container, | ||
| "version": version.Name, | ||
| }).Debugf("listing expired versions ...") |
There was a problem hiding this comment.
this is ok to be debug, but why we still listing? We probably checking them? btw formatting is not needed here, so Debug() could be used instead of Debugf()
| "version": version.Name, | ||
| }).Debugf("listing expired versions ...") | ||
|
|
||
| if version.CreatedAt.After(now.Add(-1 * container.VersionsTTL)) { |
There was a problem hiding this comment.
I would prefer to use positive filters i.e. if true ; do delete ... ; done so to log things to be deleted instead ones not going to be deleted.
This logic looks more straight forward
| log.WithFields(log.Fields{ | ||
| "namespace": namespace, | ||
| "container": container, | ||
| }).Debugf("listing unpublished versions ...") | ||
| "version": version.Name, | ||
| }).Debug("version is newer ttl. Skipping ...") |
There was a problem hiding this comment.
- If it's gonna be message about version to be deleted it probably should be info
- "version is newer than ttl"
| log.WithFields(log.Fields{ | ||
| "namespace": namespace, | ||
| "container": container, | ||
| }).Debugf("listing unpublished versions ...") |
There was a problem hiding this comment.
The same: info + it's not listing it's routine :-)
| "version": version.Name, | ||
| }).Debugf("listing unpublished expired versions ...") | ||
|
|
||
| if version.CreatedAt.After(now.Add(-1 * s.cfg.UnpublishedVersionMaxAge)) { |
There was a problem hiding this comment.
the same, positive match looks better
No description provided.