Skip to content
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

Schema Changes to match 0.7 Chip Spec #91

Merged
merged 21 commits into from Nov 2, 2021

Conversation

ankur325
Copy link
Collaborator

Added Support for Device Versions and Vendor Info.
p.s. TODO (Add some more test cases and updated documentation.)

I will add few more changes to this for documentation and test cases

Added Support for Device Versions and Vendor Info.
p.s. TODO (Add some more test cases and updated documentation.)
ankur325 and others added 3 commits October 14, 2021 12:56
docs/cli-help.md Outdated Show resolved Hide resolved
utils/conversions/conversions.go Outdated Show resolved Hide resolved
utils/conversions/conversions.go Outdated Show resolved Hide resolved
x/auth/handler_test.go Outdated Show resolved Hide resolved
x/compliance/client/cli/tx.go Show resolved Hide resolved
require.Equal(t, receivedModelVersion.SoftwareVersion, modelVersion.SoftwareVersion)
}

func TestHandler_UpdateModelVersion(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test where we create a model version with all fields, update only minimal set of fields (or no fields at all), and make sure that all fields untouched during update has initial values (are nor reset).

x/modelversion/client/rest/rest.go Outdated Show resolved Hide resolved
x/modelversion/client/rest/rest.go Outdated Show resolved Hide resolved
x/vendorinfo/genesis.go Outdated Show resolved Hide resolved
x/vendorinfo/handler_test.go Show resolved Hide resolved
@ashcherbakov
Copy link
Contributor

Thank you for these new features, improvements, and re-factorings!
This looks pretty good. We can either merge the PR as-is and do improvements/missing things in the next PR, or update this PR.

  1. As mentioned in the PR description, please update the documentation for the new command
  2. As mentioned in the PR description, please add more tests, in particular
  1. A need to specify FlagSoftwareVersionString in addition to FlagSoftwareVersion for compliance commands. See Schema Changes to match 0.7 Chip Spec #91 (comment) and Schema Changes to match 0.7 Chip Spec #91 (comment).
  2. Architecture thing: Schema Changes to match 0.7 Chip Spec #91 (comment)
  3. Other comments and suggestions

Copy link
Contributor

@ashcherbakov ashcherbakov left a comment

Choose a reason for hiding this comment

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

Looks good, let's merge the core changes.
Let's create a separate PR then with

  • more integration tests (see comments in this PR)
  • docs update (cli docs, transactions.md)

@ashcherbakov ashcherbakov merged commit 3d14596 into zigbee-alliance:master Nov 2, 2021
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.

None yet

2 participants