feat: Print detailed entity information when it already exists #204
Conversation
state/utils.go
Outdated
| @@ -13,9 +13,6 @@ const ( | |||
| // returned when an entity is not found in the state. | |||
| var ErrNotFound = errors.New("entity not found") | |||
|
|
|||
| // ErrAlreadyExists represents an entity is already present in the state. | |||
| var ErrAlreadyExists = errors.New("entity already exists") | |||
There was a problem hiding this comment.
Please add this back and use this error. We want this error to be part of the public API of this package.
state/upstream.go
Outdated
| @@ -46,7 +47,7 @@ func (k *UpstreamsCollection) Add(upstream Upstream) error { | |||
| } | |||
| _, err := getUpstream(txn, searchBy...) | |||
| if err == nil { | |||
| return ErrAlreadyExists | |||
| return errors.Errorf("upstream %v already exists", upstream.Console()) | |||
There was a problem hiding this comment.
Please instead change this to:
return fmt.Errorf("inserting upstream %v: %w", upstream.Console())This blog post and other related material on error propagation in Go might be helpful here as to why we want to do this. tldr; we want to make it easy for users of the package to know the type of error occurs here so that the the user can fail gracefully (we don't do use this error currently but we can in future).
There was a problem hiding this comment.
The same comment applies for other entities as well.
Meta: PRs like this always show that we need to remove this duplication by using code-generation or some such tool. We will eventually get to it.
There was a problem hiding this comment.
If we want to keep ErrAlreadyExists would that rather be:
return errors.Wrapf(ErrAlreadyExists, "inserting upstream %v", upstream.Console())There was a problem hiding this comment.
That and %w is essentially the same. Please read the blog post I've linked in my previous comment.
There was a problem hiding this comment.
Ah, I get it now. Thanks for the link!
file/reader.go
Outdated
| return builder.build() | ||
| state, err := builder.build() | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "error reading state file(s)") |
There was a problem hiding this comment.
Please instead change the wrap comment to "building state".
We would like to reserve read verb for Read() like methods or the read syscall.
|
@hbagdi Addressed the comments, let me know what you think. The error message now reads: $ ./deck validate -s consumers1.yaml -s consumers2.yaml
Error: error building state: inserting consumer kong: entity already exists |
file/reader.go
Outdated
| return builder.build() | ||
| state, err := builder.build() | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error building state") |
There was a problem hiding this comment.
Can we change the wrap string to building state?
As you can see in the output, error is printed twice, which is redundant.
Currently, when an entity is duplicated in the state file(s) the error message is not very helpful, e.g.:
In a large and distributed configuration, it is often not easy to figure out which entity this is. Also, the error message itself does not say where the entity already exists - it could mean exists in Kong.
With this PR, the error message becomes a lot more helpful:
This applies across
deck sync,deck diff, anddeck validate.