Closed q-uint closed 6 months ago
Here's a PR that with a solution I propose regarding the logger: #170
Thanks for taking the time to go though this @icamys! :)
@q-uint My pleasure! I am happy to work on improving this package.
@icamys not sure why the tests are failing, I'll try to look at it later today.
@q-uint Thank you for merging the logger-related changes.
I have a proposal regarding the release strategy. Currently, when you run go get github.com/elimity-com/scim
, you always get the latest version of the repo from the main branch, which is df55fde at the moment. Since this PR changes how the server is created, a package upgrade command invocation will probably break the existing builds for other devs using this package: they'll either have to update the Server construction in their code or roll back to the package version with which their applications work. For those who want to roll back, there won't be a release version to attach to, as they previously pulled just the latest version.
I propose to create a release for the currently latest commit in the main branch and name it, for example, 1.0
, to allow people who want to use the old package version to roll back and attach to a release version. After this PR is merged into the main branch, create a new release, for example, 2.0
, to allow those who want to use the newer version and upgrade their code to attach to 2.x
versions.
I will make sure to add tags. I will not make it a major version yet, since it is not feature complete. It will probably be 0.1
and 0.2.0
.
@q-uint I'm ok with any approach, as I plan to use this package after this merge. Still, the issue about incrementing the minor version is that the package upgrade command go get -u package_name
will upgrade to the latest minor or patch version according to the docs, which will break existing builds:
The -u flag instructs get to update modules providing dependencies of packages named on the command line to use newer minor or patch releases when available.
We are still sub version 1, so people should upgrade with caution. An increase in minor version can mean non backwards compatible changes. The moment we release v1
we would not be able to do that anymore.
@icamys if you run the tests locally, do they also pass? I can not replicate the behavior of the CI on my machine.
@q-uint It also works fine on my machine. It is worth trying to create a new branch out of this one and push it to remote to trigger CI for it.
I saw that you changed back the server creation functions in the tests from getNewServer(t *testing.T) Server
to getNewServer() (Server, error)
: there are a couple of instances left in:
Hi!
Github is running tests by checking out a commit equivalent to the merge between the PR and its base branch.
You can reproduce them by merging master
inside qol
before running tests, or by using the github-provided "merge" reference as such:
$ git fetch origin refs/pull/169/merge
$ git checkout FETCH_HEAD
Then, the line numbers will make more sense, as there are some changes in master that reference newTestServer()
without expecting it to return an error value, causing the build to fail once both branches are merged together.
Hi @mrene thanks for jumping in, totally forgot we were one commit behind on main! Thanks 😉
Please, feel free to review/comment! I will probably merge this Tuesday morning (CET).
Thanks for the feedback! 🏆
Hi @q-uint ! I will review it tomorrow morning; thank you! Also, kudos to @mrene for explaining the issue.
json.Number
bug.