Various QOL changes.

[q-uint]

• Upgrade CI.
• Add option to allow string values.
  • Fixes Azure's noncompliance of standard #163
  • Closes Allow strings as numbers and booleans #165
• Fix Patch json.Number bug.
  • Closes allow patch operations to validate integer subattributes correctly #159
  • Closes Use json.Number to decode patch operations #164
• Add custom logger for internal errors.
  • Closes Add logging facility. #147
  • Fixes Proposal: add custom logger support #168
• Fix getters in schema pkg.
• Move filter package to root.
  • Closes Publish filters package #144
• Expose filter validator.
  • Closes ListRequestParams: expose validator instead of just plain filter #146

[icamys]

Here's a PR that with a solution I propose regarding the logger: #170

[q-uint]

Thanks for taking the time to go though this @icamys! :)

[icamys]

@q-uint My pleasure! I am happy to work on improving this package.

[q-uint]

@icamys not sure why the tests are failing, I'll try to look at it later today.

[icamys]

@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.

[q-uint]

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.

[icamys]

@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.

[q-uint]

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.

[icamys]

@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:

• internal/idp_test/util_test.go
• internal/idp_test/okta_util_test.go
• internal/idp_test/azuread_util_test.go

[mrene]

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.

[q-uint]

Hi @mrene thanks for jumping in, totally forgot we were one commit behind on main! Thanks 😉

[q-uint]

Please, feel free to review/comment! I will probably merge this Tuesday morning (CET).

Thanks for the feedback! 🏆

[icamys]

Hi @q-uint ! I will review it tomorrow morning; thank you!
Also, kudos to @mrene for explaining the issue.

[icamys]

Hi @q-uint ! I left a couple of comments related to the documentation and the Makefile. The code itself looks good.

[icamys]

Since the Makefile has been added, wouldn't using it in the Github workflow make sense? There is some code duplication now in both places.

[q-uint]

There is some duplication, the big difference is that in the CI it checks whether it has been done, and in the Makefile it is actually executing the actions.

[icamys]

LGTM. Thank you!