Copilot commented on code in PR #846:
URL: https://github.com/apache/solr-operator/pull/846#discussion_r4514359922
##########
controllers/util/backup_util.go:
##########
@@ -124,6 +124,12 @@ func CheckBackupForCollection(ctx context.Context, cloud
*solr.SolrCloud, collec
finished = true
success = false
}
+ if asyncStatus == "notfound" {
+ finished = true
+ success = false
+ logger.Info("Solr has no record of the collection
backup, so it cannot be confirmed. Marking it unsuccessful; the backup
repository may still contain a usable backup.",
+ "solrCloud", cloud.Name, "collection",
collection)
+ }
Review Comment:
The status handling is now a growing chain of independent `if` statements
comparing string literals. Converting this to a `switch asyncStatus` (or `else
if` chain) makes exclusivity explicit and reduces the chance of accidental
double-handling as more states are added. Also consider centralizing
`"completed"`, `"failed"`, and `"notfound"` into constants (ideally shared with
`solr_api`) to avoid typo-prone string duplication.
##########
controllers/util/backup_util_test.go:
##########
@@ -413,3 +427,51 @@ func
TestRepositoryLookupFailsIfNoNameProvidedAndMultipleRepositoriesDefined(t *
assert.Nil(t, found, "Expected GetBackupRepositoryByName to report no
match")
}
+
+// A backup whose async status Solr no longer holds must be terminal,
otherwise it is checked
+// forever. It is unsuccessful because success cannot be shown, not because
failure is known.
+func TestCheckBackupForCollectionOutcome(t *testing.T) {
+ tests := map[string]struct {
+ finished bool
+ success bool
+ }{
+ "completed": {finished: true, success: true},
+ "failed": {finished: true, success: false},
+ "notfound": {finished: true, success: false},
+ "running": {finished: false, success: false},
+ "submitted": {finished: false, success: false},
+ }
+
+ defer func() {
+ transport := http.DefaultTransport.(*http.Transport).Clone()
+ transport.TLSClientConfig = &tls.Config{InsecureSkipVerify:
true}
+ solr_api.SetNoVerifyTLSHttpClient(&http.Client{Transport:
transport})
+ }()
+
+ for asyncStatus, expected := range tests {
+ solr_api.SetNoVerifyTLSHttpClient(&http.Client{Transport:
roundTripFunc(func(request *http.Request) (*http.Response, error) {
+ assert.Equal(t, "REQUESTSTATUS",
request.URL.Query().Get("action"))
+ assert.Equal(t, "backup-collection",
request.URL.Query().Get("requestid"))
+ return &http.Response{
+ StatusCode: http.StatusOK,
+ Header: make(http.Header),
+ Body:
io.NopCloser(strings.NewReader(fmt.Sprintf(
+
`{"responseHeader":{"status":0},"status":{"state":%q}}`, asyncStatus,
+ ))),
+ }, nil
+ })})
+
+ finished, success, actualStatus, err :=
CheckBackupForCollection(
+ context.Background(),
+ &solr.SolrCloud{ObjectMeta: metav1.ObjectMeta{Name:
"cloud", Namespace: "namespace"}},
+ "collection",
+ "backup",
+ logr.Discard(),
+ )
+
+ assert.NoError(t, err)
+ assert.Equal(t, asyncStatus, actualStatus)
+ assert.Equal(t, expected.finished, finished, "Wrong finished
for async status %q", asyncStatus)
+ assert.Equal(t, expected.success, success, "Wrong success for
async status %q", asyncStatus)
Review Comment:
This test mutates a global HTTP client in `solr_api` (multiple times inside
the loop) without restoring the previous client value. That can leak state into
other tests and introduces flakiness if other tests run in parallel within the
package (or if `t.Parallel()` is added later). Prefer capturing/restoring the
previous client via a getter (or adding one), or avoid global mutation by
injecting the HTTP client/transport into the code under test; at minimum,
restore the original client with `t.Cleanup` to prevent cross-test interference.
##########
controllers/util/backup_util_test.go:
##########
@@ -413,3 +427,51 @@ func
TestRepositoryLookupFailsIfNoNameProvidedAndMultipleRepositoriesDefined(t *
assert.Nil(t, found, "Expected GetBackupRepositoryByName to report no
match")
}
+
+// A backup whose async status Solr no longer holds must be terminal,
otherwise it is checked
+// forever. It is unsuccessful because success cannot be shown, not because
failure is known.
+func TestCheckBackupForCollectionOutcome(t *testing.T) {
+ tests := map[string]struct {
+ finished bool
+ success bool
+ }{
+ "completed": {finished: true, success: true},
+ "failed": {finished: true, success: false},
+ "notfound": {finished: true, success: false},
+ "running": {finished: false, success: false},
+ "submitted": {finished: false, success: false},
+ }
+
+ defer func() {
+ transport := http.DefaultTransport.(*http.Transport).Clone()
+ transport.TLSClientConfig = &tls.Config{InsecureSkipVerify:
true}
+ solr_api.SetNoVerifyTLSHttpClient(&http.Client{Transport:
transport})
+ }()
+
+ for asyncStatus, expected := range tests {
+ solr_api.SetNoVerifyTLSHttpClient(&http.Client{Transport:
roundTripFunc(func(request *http.Request) (*http.Response, error) {
+ assert.Equal(t, "REQUESTSTATUS",
request.URL.Query().Get("action"))
+ assert.Equal(t, "backup-collection",
request.URL.Query().Get("requestid"))
+ return &http.Response{
+ StatusCode: http.StatusOK,
+ Header: make(http.Header),
+ Body:
io.NopCloser(strings.NewReader(fmt.Sprintf(
+
`{"responseHeader":{"status":0},"status":{"state":%q}}`, asyncStatus,
+ ))),
+ }, nil
+ })})
+
+ finished, success, actualStatus, err :=
CheckBackupForCollection(
+ context.Background(),
+ &solr.SolrCloud{ObjectMeta: metav1.ObjectMeta{Name:
"cloud", Namespace: "namespace"}},
+ "collection",
+ "backup",
+ logr.Discard(),
+ )
+
+ assert.NoError(t, err)
+ assert.Equal(t, asyncStatus, actualStatus)
+ assert.Equal(t, expected.finished, finished, "Wrong finished
for async status %q", asyncStatus)
+ assert.Equal(t, expected.success, success, "Wrong success for
async status %q", asyncStatus)
Review Comment:
`assert.Equal`’s `msgAndArgs` is not printf-formatted in testify; passing a
format string plus args won’t substitute `%q`, leading to confusing failure
output. Use `assert.Equalf(...)` (or build the message with `fmt.Sprintf`) so
the asyncStatus is interpolated correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
