-
Notifications
You must be signed in to change notification settings - Fork 75
Update all deps to latest where possible, bump Kube deps to v0.23.1 #914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
af5882a to
b1f5e67
Compare
Codecov Report
@@ Coverage Diff @@
## main #914 +/- ##
==========================================
+ Coverage 79.00% 79.04% +0.03%
==========================================
Files 133 133
Lines 9776 9771 -5
==========================================
- Hits 7724 7723 -1
+ Misses 1789 1785 -4
Partials 263 263
Continue to review full report at Codecov.
|
| issuerEndpointPtr = &issuerEndpoint | ||
|
|
||
| testLog := testlogger.New(t) | ||
| testLog := testlogger.NewLegacy(t) //nolint: staticcheck // old test with lots of log statements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned on a call that the changes to the logs are pretty small and inconsequential, so it makes more sense to just continue using the old format rather than changing hundreds of test assertions. How will we know if in the future there is a change that's more consequential that we should pick up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned on a call that the changes to the logs are pretty small and inconsequential
Specifically, some white space changes in the logs and the ordering of keys is slightly different now. More than enough to break all of our test assertions in a way that is non-trivial to fix.
so it makes more sense to just continue using the old format rather than changing hundreds of test assertions.
Only for old, preexisting tests.
How will we know if in the future there is a change that's more consequential that we should pick up?
Two things:
- I marked
testlogger.NewLegacyas deprecated so our linter will get upset if it is used. This is to discourage its use in new tests (they should usetestlogger.Newinstead). This will give us coverage for both the old and new format. - None of this actually matters for the real logs our code emits because those logs use the
klogformat, which is distinctly different from thestdrlog format. I do not know why we chose this format for our tests, but it is what we have today (and it does assert that certain logs happened at certain times). As part of the audit work, I would like to migrate this stuff in a way that it is consistent with the real logs being emitted by our code (which should be some kind of JSON format at that point).
b1f5e67 to
956d3c6
Compare
Highlights from this dep bump: 1. Made a copy of the v0.4.0 github.com/go-logr/stdr implementation for use in tests. We must bump this dep as Kube code uses a newer version now. We would have to rewrite hundreds of test log assertions without this copy. 2. Use github.com/felixge/httpsnoop to undo the changes made by ory/fosite#636 for CLI based login flows. This is required for backwards compatibility with older versions of our CLI. A separate change after this will update the CLI to be more flexible (it is purposefully not part of this change to confirm that we did not break anything). For all browser login flows, we now redirect using http.StatusSeeOther instead of http.StatusFound. 3. Drop plog.RemoveKlogGlobalFlags as klog no longer mutates global process flags 4. Only bump github.com/ory/x to v0.0.297 instead of the latest v0.0.321 because v0.0.298+ pulls in a newer version of go.opentelemetry.io/otel/semconv which breaks k8s.io/apiserver. We should update k8s.io/apiserver to use the newer code. 5. Migrate all code from k8s.io/apimachinery/pkg/util/clock to k8s.io/utils/clock and k8s.io/utils/clock/testing 6. Delete testutil.NewDeleteOptionsRecorder and migrate to the new kubetesting.NewDeleteActionWithOptions 7. Updated ExpectedAuthorizeCodeSessionJSONFromFuzzing caused by fosite's new rotated_secrets OAuth client field. This new field is currently not relevant to us as we have no private clients. Signed-off-by: Monis Khan <mok@vmware.com>
956d3c6 to
9599ffc
Compare
Highlights from this dep bump:
for use in tests. We must bump this dep as Kube code uses a
newer version now. We would have to rewrite hundreds of test log
assertions without this copy.
fix: force HTTP GET for redirect responses ory/fosite#636 for CLI based login flows. This is required for
backwards compatibility with older versions of our CLI. A
separate change after this will update the CLI to be more
flexible (it is purposefully not part of this change to confirm
that we did not break anything). For all browser login flows, we
now redirect using http.StatusSeeOther instead of http.StatusFound.
process flags
v0.0.321 because v0.0.298+ pulls in a newer version of
go.opentelemetry.io/otel/semconv which breaks k8s.io/apiserver.
We should update k8s.io/apiserver to use the newer code.
k8s.io/utils/clock and k8s.io/utils/clock/testing
kubetesting.NewDeleteActionWithOptions
fosite's new rotated_secrets OAuth client field. This new field
is currently not relevant to us as we have no private clients.
Signed-off-by: Monis Khan mok@vmware.com
Release note: