SDK server not clearing lists on update #3606
Conversation
|
Build Succeeded 👏 Build Id: b78bf70d-80aa-4a27-b33e-573c64133a00 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 9e3f7f6d-e01b-4797-aa58-173ccd859a80 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
| // Clear the gsCounterUpdates | ||
| s.gsCounterUpdates = map[string]counterUpdateRequest{} | ||
| // Clear the gsListUpdates | ||
| s.gsListUpdates = map[string]listUpdateRequest{} |
There was a problem hiding this comment.
Do we need an update to a unit test to ensure this doesn't happen again?
There was a problem hiding this comment.
I'll have a look in the unit test file to see what's doable.
There was a problem hiding this comment.
Appreciate you taking a look.
No rush, but just a heads up - we're cutting a release next Tuesday, so if you want this in the next release, we should get some unit tests in.
@igooch any advice on a good spot to write a unit test?
There was a problem hiding this comment.
I have something actually but it's at work, so tomorrow I should send an update to this MR, idea is:
- add a function that get the len() of the outgoing lists
- happy path = len() == 0
- network failure / issues updating = original len
There was a problem hiding this comment.
Looks good! The only thing I'd add is checking that the gsListUpdates has the update, i.e. is not zero after the update/add/remove call and before the get list call. Something like
if testCase.updated {
updates := sc.gsListUpdatesLen()
assert.Equal(t, 1, updates)
}
There was a problem hiding this comment.
^Scratch that above suggestion, it would introduce a race condition. Code looks good as it stands.
|
Build Succeeded 👏 Build Id: 63226160-64fe-4be2-815f-1ecc42098740 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 2c1b7e2e-dc12-4ef0-bb36-20bc3fafc2aa The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind bug
What this PR does / Why we need it:
Fixes the issue where items in list could not be removed.
Which issue(s) this PR fixes:
#3593
Special notes for your reviewer:
Had to recreate the PR because of CLA issues.