Upgrade zap to v1.21.0 #42
Conversation
|
💚 CLA has been signed |
|
/sign |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
@alex1989hu what is the reason for the module upgrade? Without a good reason, I would prefer not to break compatibility with older versions of Go. Users of ecszap can still use a newer version of zap without forcing it in ecszap's go.mod. |
hey @axw probably the best reason is to remediate vulnerabilities which can easily end up in the user code due to outdated
If the user's code depends on On the other hand, if I am not mistaken, Go 1.13 is already reached its end of life. I also found Go 1.15 is also outdated but Worth to consider that
|
|
Understood. Users should update to the latest Go version, and the latest zap version, to get security bug fixes and enhancements. However, it is not ecszap's responsibility to enforce this -- it's the application developer's. If ecszap were a logger in its own right which just happened to use zap undder the hood, it would be a different matter; but ecszap is a companion to zap, so it's reasonable to expect application developers to keep their zap dependency up-to-date. Re Go versions: our stance is that the supported Go versions forms part of the semver contract. This isn't followed by all Go libraries, but it is followed by ecszap. Updating the minimum supported version would require a major version bump, and I don't think that's warranted at the moment. Again, application developers are responsible for updating their Go version. |
I would like to ask you to clarify the gap between maintaining dependency of I think I am confused. If we agree that is the application developer responsibility to keep their dependant library up-to-date that all the following PRs shall be not merged:
Related to semver contract concern: to reflect breaking change I propose to release v2 module of |
|
Fair questions! You're correct that ecs-logging-go-zap and ecs-logging-java are equivalent in this regard. I was perhaps being too extreme. Evidently we do sometimes bump versions for vulnerabilities, log4j2 being a great case in point. That was a critical vulnerability, and we did everything we could to limit its impact. Fortunately that could be done without breaking changes. If we couldn't, we would have done what you suggest and create new major version. I just tested bumping ecszap's zap dependency to v1.21.0, along with Go 1.13, and it works fine. Zap may not support officially support <Go1.15, but I think we can bump without also bumping the minimum supported version of Go for ecszap. How does that sound do you? |
|
Thank you for the clarification. I can accept your quick testing experience related to Go 1.13 I am happy to rewrite the commit - reverting Go 1.13 related changes - in this PR if we can agree upon this. Almost forget to mention: if this (modified) PR is in, could you please create tag to let application developers use the dependency with a proper tag. I know, we can use commit SHA1 as library version but having a tag would be nicer. |
Sounds good, please go ahead and revert the "go 1.15" change in go.mod and and the Jenkinsfile change, and I'll be happy to merge.
No problem, I can create a patch release once this is merged. |
This upgrade also implies that we have to build on Go 1.15 which is build requirement of recent zap version However - as per PR discussion - we continue to build the library with Go 1.13 Signed-off-by: Alex Szakaly <alex.szakaly@gmail.com>
|
@axw I have just updated the PR |
|
/test |
|
Thanks @alex1989hu! I'll create a new release soon. |
This upgrade also implies that we have to build
on Go 1.15 which is build requirement of recent
zap version
However - as per PR discussion - we continue to
build the library with Go 1.13