Skip to content

Extend box information with replication fields #427

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

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

w33dw0r7d
Copy link

What has been done? Why? What problem is being solved?

Extend box information with replication fields.

I didn't forget about (remove if it is not applicable):

@w33dw0r7d w33dw0r7d force-pushed the add-box-info-replication branch 3 times, most recently from 0b8374e to a0325d8 Compare January 15, 2025 10:04
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch!

Please, add tests for the change (integration tests to the box/tarantool_test.go and a unit-tests of the message pack decoding in info_test.go).

@w33dw0r7d w33dw0r7d force-pushed the add-box-info-replication branch from a0325d8 to 5d91dde Compare January 15, 2025 14:52
@w33dw0r7d
Copy link
Author

@oleg-jukovec Thanks for the quick response.
I add integration test and add pull request number in changelog.

unit-tests of the message pack decoding in info_test.go

Please tell me what you think needs to be tested in this test.

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Jan 15, 2025

Please tell me what you think needs to be tested in this test.

You could encode a map as a MessagePack and decode it as InfoResponse. The pseudo-code:

expected := InfoResponse{
     Info: Info{
         Foo: "bar",
     },
}
data, err := msgpack.Marshal(map[string]any{
     "foo": "bar",
})
require.NoError(err)

var result InfoResponse
err = msgpack.Unmarshal(data, &result)
require.NoError(err)

require.Equals(t, expected, result)

In that way you could ensure that all fields decoded as expected and you don't make a mistake or misprint. It also would be a good starting point for tests in the future.

You need to cover decoding of all new fields with the test.

@w33dw0r7d w33dw0r7d force-pushed the add-box-info-replication branch from 5d91dde to 153fc25 Compare January 16, 2025 10:07
@w33dw0r7d
Copy link
Author

@oleg-jukovec Thanks for your detailed answer. I add tests. Could you please take a look?

@w33dw0r7d w33dw0r7d force-pushed the add-box-info-replication branch from 153fc25 to 8d0d862 Compare January 16, 2025 11:58
@w33dw0r7d
Copy link
Author

Add additional fields .message .system_message.

@w33dw0r7d w33dw0r7d force-pushed the add-box-info-replication branch 2 times, most recently from e95cce5 to 1a78e05 Compare January 16, 2025 13:22
@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Jan 16, 2025

Please, fix linter errors:

  Error: box/info_test.go:63:1: The line is 106 characters long, which exceeds the maximum of 100 characters. (lll)
  							Message:       "'getaddrinfo: Name or service not known: Input/output error'",
  ^
  Error: box/info_test.go:100:1: The line is 109 characters long, which exceeds the maximum of 100 characters. (lll)
  							"message":        "'getaddrinfo: Name or service not known: Input/output error'",
  ^
  
  Error: issues found

@w33dw0r7d w33dw0r7d force-pushed the add-box-info-replication branch from 1a78e05 to f0013af Compare January 17, 2025 09:49
@w33dw0r7d
Copy link
Author

@oleg-jukovec For some unknown reason local golangci-lint doesn't work. Fix all problems.

Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch!

@oleg-jukovec oleg-jukovec requested a review from DerekBum January 17, 2025 10:28
Copy link

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch, just some small nits.

@w33dw0r7d w33dw0r7d force-pushed the add-box-info-replication branch from f0013af to f3d9e7b Compare January 17, 2025 11:10
@w33dw0r7d
Copy link
Author

@DerekBum Thanks for you review.

@oleg-jukovec oleg-jukovec merged commit d8e2284 into tarantool:master Jan 17, 2025
20 checks passed
@oleg-jukovec
Copy link
Collaborator

Thank you for the patch!

@oleg-jukovec oleg-jukovec mentioned this pull request Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants