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

Add gRPC interface function to write settings to file #1144

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

silvanocerza
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

Adds a new function to the gRPC interface.

  • What is the current behavior?

There is currently no way to update the config file from the gRPC interface.

  • What is the new behavior?

Consumers of the gRPC interface can now call a function Write(*rpc.WriteRequest) to write settings currently stored in memory to the file specified in rpc.WriteRequest.

Nope.

  • Other information:

None.


See how to contribute

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

How about a demo in the client_example?

diff --git a/client_example/main.go b/client_example/main.go
index 4a6164b7..08cf1c43 100644
--- a/client_example/main.go
+++ b/client_example/main.go
@@ -90,6 +90,10 @@ func main() {
        log.Println("calling GetValue(foo)")
        callGetValue(settingsClient)

+       // Write settings to file.
+       log.Println("calling Write()")
+       callWrite(settingsClient)
+
        // Before we can do anything with the CLI, an "instance" must be created.
        // We keep a reference to the created instance because we will need it to
        // run subsequent commands.
@@ -256,6 +260,17 @@ func callGetAll(client settings.SettingsClient) {
        log.Printf("Settings: %s", getAllResp.GetJsonData())
 }

+func callWrite(client settings.SettingsClient) {
+       _, err := client.Write(context.Background(),
+               &settings.WriteRequest{
+                       FilePath: path.Join(dataDir, "written-settings.yml"),
+               })
+
+       if err != nil {
+               log.Fatalf("Error writing settings: %s", err)
+       }
+}
+
 func initInstance(client rpc.ArduinoCoreClient) *rpc.Instance {
        // The configuration for this example client only contains the path to
        // the data folder.

@silvanocerza
Copy link
Contributor Author

How about a demo in the client_example?

Yeah, that's a good idea. Sooner than later I want to ditch that client_example for a set of structured integration tests though.

Soon. 😢

@per1234
Copy link
Contributor

per1234 commented Jan 20, 2021

I want to ditch that client_example for a set of structured integration tests though

I'm all for adding integration tests, but the purpose of client_example is documentation. I found it to be extremely helpful when I was documenting the gRPC interface. I don't think integration tests could serve as well as documentation. So I would like to see the client_example remain even after the tests are added.

@silvanocerza
Copy link
Contributor Author

I want to ditch that client_example for a set of structured integration tests though

I'm all for adding integration tests, but the purpose of client_example is documentation. I found it to be extremely helpful when I was documenting the gRPC interface. I don't think integration tests could serve as well as documentation. So I would like to see the client_example remain even after the tests are added.

Ideally well written tests can act as documentation but I get your concerns.

@silvanocerza silvanocerza merged commit 6328430 into master Jan 20, 2021
@silvanocerza silvanocerza deleted the scerza/config-grpc-write branch January 20, 2021 16:52
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

3 participants