Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #13877 +/- ##
==========================================
- Coverage 58.76% 58.76% -0.01%
==========================================
Files 899 900 +1
Lines 73855 74009 +154
Branches 2098 2098
==========================================
+ Hits 43401 43489 +88
- Misses 26996 27047 +51
- Partials 3458 3473 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| _, err := tx.Exec("SET SESSION sort_buffer_size = 2560000000") | ||
| if err != nil { | ||
| return fmt.Errorf("increasing global sort buffer size: %w", err) | ||
| } |
There was a problem hiding this comment.
I was getting:
Error 1038 (HY001): Out of sort memory, consider increasing server sort buffer size
on the newly added check for rows.Err below. I figured this is the simplest way to go about this without touching this tricky migration too much, but I'm open to any ideas.
| main: | ||
| deny: | ||
| - pkg: github.com/pkg/errors | ||
| msg: "use ctxerr if a context.Context is available or stdlib errors.New / fmt.Errorf with the %w verb" |
There was a problem hiding this comment.
the configuration changed in this newer version, so I adjusted it accordingly.
georgekarrv
left a comment
georgekarrv
left a comment
There was a problem hiding this comment.
LGTM, we should try to merge this asap so devs can get some runtime with it applied before release
| } | ||
| secretsCreatedAt := make(map[string]*time.Time, len(existingSecrets)) | ||
| for _, es := range existingSecrets { | ||
| es := es |
There was a problem hiding this comment.
it might! I'm not completely sure. How cool is that new check?
There was a problem hiding this comment.
This is an awesome check. The issue is that we were taking the address of es.CreatedAt in the loop, but es gets modified on each loop iteration, so that pointer pointed to the last CreatedAt in the existingSecrets slice. Unlikely to have been a big issue (since this is just the created-at timestamp), but still...
| {"unenroll after 3 tries", nil, 3, false}, | ||
| {"unenroll after one try", nil, 1, false}, | ||
| {"error after max number of tries is exceeded", nil, 99, true}, | ||
| {"error after max number of tries is exceeded", nil, 1000, true}, |
There was a problem hiding this comment.
Any reason this was bumped so much?
There was a problem hiding this comment.
I wanted to be on the safe side, but you made me wonder if I'm not hiding some dirt under the rug here. I will circle back to this between today and tomorrow.
There was a problem hiding this comment.
Oh was that a flaky test previously?
|
|
||
| // Load targets | ||
| for _, spec := range specs { | ||
| spec := spec |
There was a problem hiding this comment.
Yeah those were fine since we don't store the address (it gets used inside the loop before the next iteration), but makes sense that it's a bit much to ask of the lint check to go to that level of analysis.
related to #13715, this updates the infra dependencies for the general go upgrade that's happening at #13877 I'm thinking we probably want to merge this after we're sure that everything works well, maybe even after we have a release with go 1.21.1 out, so I'm making a separate PR. --------- Co-authored-by: Martin Angers <martin.n.angers@gmail.com>
For #16795, this: - Updates Go to go1.22.3 - Per #16795 (comment), I also ran the following to update the versions requested by @getvictor ``` go get github.com/kataras/golog@v0.1.12 go get github.com/kataras/iris/v12@v12.2.11 go get github.com/sethvargo/go-password@v0.3.0 ``` **Notes** After this is merged people will need to update their Go version. I use gvm and I did it like: ``` $ gvm install go1.22.3 $ gvm use go1.22.3 --default ``` **Relevant changes** The release notes mention: > Previously, the variables declared by a “for” loop were created once > and updated by each iteration. In Go 1.22, each iteration of the loop > creates new variables, to avoid accidental sharing bugs. However, we already have a lint rule (see #13877) for this scenario, so it shouldn't affect us.
For #16795, this: - Updates Go to go1.22.3 - Per #16795 (comment), I also ran the following to update the versions requested by @getvictor ``` go get github.com/kataras/golog@v0.1.12 go get github.com/kataras/iris/v12@v12.2.11 go get github.com/sethvargo/go-password@v0.3.0 ``` **Notes** After this is merged people will need to update their Go version. I use gvm and I did it like: ``` $ gvm install go1.22.3 $ gvm use go1.22.3 --default ``` **Relevant changes** The release notes mention: > Previously, the variables declared by a “for” loop were created once > and updated by each iteration. In Go 1.22, each iteration of the loop > creates new variables, to avoid accidental sharing bugs. However, we already have a lint rule (see #13877) for this scenario, so it shouldn't affect us.
5 participants
For #13715, this:
1.21.1, infrastructure changes are addressed separately at upgrade Go to 1.21.1 for infra #13878After this is merged people will need to:
gvmand I did it like:golangci-lint:Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/ororbit/changes/.See Changes files for more information.