feat(release): Issue #45 add support for command chaining #368

nicolasjhampton · 2020-08-31T06:47:54Z

What

This PR adds a pipe module to read stdin for piping input to commands, allowing each command
be piped to the next. I've also implemented this pipe logic for one command, the entity tags get
command. You can try this functionality by running:

newrelic entity search --name [app name] | newrelic entity tags get

Next work needed

Currently the above action only returns tags for the first entity found. Work has to be done in
newrelic-client-go to add bulk queries to the API. Specifically for this tag query, something like this query needs
to be added (this is incorrect, working on it in a different repo):

query($guids:[EntityGuid]!) {
  actor {
    entities(guids: $guids) {
        tags {
          values
          key
        }
      }
    }
  }
}

For the entity tag command alone, nrClient.Entities.ListTags, nrClient.Entities.DeleteTags, nrClient.Entities.DeleteTags, nrClient.Entities.DeleteTagValues, nrClient.Entities.AddTags, and nrClient.Entities.ReplaceTags all need bulk versions written that can take multiple entity guids.

Documentation

Doc comments included for all of the Public and some of the private API

#45

CLAassistant · 2020-08-31T06:47:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

zlesnr · 2020-08-31T15:37:47Z

Oh interesting, thanks for putting this up @nicolasjhampton. One idea that @sanderblue and I talked about was having a "mother" parent object that would nest each of the kinds of objects we expect to want to take over STDIN. In this way, the shape of the received object could change, and each command would know what it would need to expect. I didn't play with this idea to know if it would work though.

nicolasjhampton · 2020-08-31T18:04:42Z

Oh interesting, thanks for putting this up @nicolasjhampton. One idea that @sanderblue and I talked about was having a "mother" parent object that would nest each of the kinds of objects we expect to want to take over STDIN. In this way, the shape of the received object could change, and each command would know what it would need to expect. I didn't play with this idea to know if it would work though.

@zlesnr That sounds like it's necessary for encoding/json and most parsers in go. I did the same thing, just without the struct list. I got around needing to know the shape by finding a parser that didn't require a struct that knows the shape of the input. That way, as long as it's valid json in the stdin with the attribute you need at the first level of the object, the command can read only the attributes it needs, never having to know the full shape of the data coming in. https://github.com/tidwall/gjson

nicolasjhampton · 2020-08-31T18:16:49Z

If this is a good general direction and there's still buy-in for this work, I'd like to clean up some code, make another helper method or two, add more error handling and tests, and finish out the entity commands as a proof of concept.

zlesnr

This is really neat. I checked out the code to play with it a bit, and I like what you're doing here. Some of the methods are hard to read, or to know what they do in the schema of things, and so at least I'd think they need some documentation about their purpose. I left a few comments, so let me know what you think.

zlesnr · 2020-09-18T17:50:41Z

internal/pipe/pipe_test.go

+	}
+
+	for _, c := range cases {
+		input := c["input"]


I'd suggest that the test cases here be a struct rather than a map. Then we can refer to fields on the struct for the expected test cases, and it's a little cleaner to read.

For an example, you can see the test struct here: https://github.com/newrelic/tutone/blob/master/internal/schema/schema_test.go#L19

That one remains a little ugly due to the whitespace handling that is part of the test, so indenting has to be specific, but you can get the idea from the struct there.

I like that, then the test cases can be named. I'll work on that.

zlesnr · 2020-09-18T17:53:10Z

internal/entities/command_tags.go

-			utils.LogIfFatal(err)
-
-			utils.LogIfError(output.Print(tags))
+			// Temporary until bulk actions can be build into newrelic-client-go


What work needs to be one in the client so that we can avoid the temporary but here?

Most of the API in the newrelic-client-go library aren't designed to deal with multiple entity guids. In this tag command alone, bulk entity commands for nrClient.Entities.ListTags, nrClient.Entities.DeleteTags, nrClient.Entities.DeleteTagValues, nrClient.Entities.AddTags, and nrClient.Entities.ReplaceTags need to be created.

A yes, methods that will take a list of guids to operate on.

internal/entities/command_tags.go

zlesnr · 2020-09-18T18:12:31Z

internal/pipe/pipe.go

+
+var pipeInput map[string][]string = map[string][]string{}
+
+func Get(inputKey string) ([]string, bool) {


This method looks to be called once unless I overlooked another place. What do you think about folding this logic into the Exists() method?

The Get method would be called everytime a command needs to get a value. It's called once because I've only used this module on one command. It's just good, clean, self-explanatory API design.

zlesnr · 2020-09-18T18:18:30Z

internal/pipe/pipe.go

+
+var GetInput = getPipeInputFactory(stdinPipeReader{input: os.Stdin}, pipeInputExists)
+
+var pipeInput map[string][]string = map[string][]string{}


The logic that populates this variable is a little hard to read. If multiple getPipeInputFactory functions are created, the calls to those methods would potentially clobber this variable and then anything relying on it might go a little pear shaped. Even in the case of GetInput() being called with different arguments I think would change the content of this variable.

getPipeInputFactory is a lowercase function, meaning it's private to this module and can't be called outside of it. The only time it's ran is when this module is loaded. I do see how GetInput could be ran twice to alter pipeInput, however. I'll add code to prevent pipeInput from being overridden if GetInput is ran twice.

This PR adds utility methods and logic to read stdin for details needed in commands, allowing each command ouput to be piped to the next command newrelic#45 refactor(release): WIP clean up logic in init function This commit refactors the init logic to be cleaner, handle more flags, and prepare for bulk actions feat(release): WIP collect values from multiple results and add tests This commit adds more testing and allows reading values from multiple results refactor(release): WIP create pipe module This commit pulls the pipe input logic out of the utils module and into a new pipe module. It also abstracts the PipeInput map from other modules. feat(release): make newrelic entity tags get cmd take stdin This commit is the last step I can take towards making the entity tags get command return bulk data based on stdin. Work has to be done on newrelic-client-go in order to support bulk guid queries https://github.com/newrelic/newrelic-client-go/blob/13ed6ee7fa172cce9218cf30e98eab6e1805541d/pkg/entities/tags.go#L124 fix(release): make Get and GetInput always return same values This commit checks for the existence of the pipeInput internal state, so even if GetInput is called more than once, nothing is overwritten and Get returns the same values. More testing and test refactors are also included. docs(release): add docs comments to pipe package docs(release): fix spelling errors

zlesnr

This looks great. Thanks for adding some method docs too! Let me know when its no longer WIP and I'll run the tests locally before merge.

zlesnr · 2020-09-21T16:12:00Z

internal/pipe/pipe_test.go

+	}
+
+	for _, c := range cases {
+		mockStdin := strings.NewReader(c.Input)


This is neat.

zlesnr · 2020-09-21T17:02:15Z

The tests passed in #380. Merging.

nicolasjhampton force-pushed the feat/pipe-std-to-entity-tag-cmd branch from 1842472 to db3b4c5 Compare September 1, 2020 20:47

zlesnr suggested changes Sep 18, 2020

View reviewed changes

nicolasjhampton force-pushed the feat/pipe-std-to-entity-tag-cmd branch 3 times, most recently from fc41f15 to e6a8b25 Compare September 20, 2020 04:41

zlesnr approved these changes Sep 21, 2020

View reviewed changes

internal/pipe/pipe_test.go

}

for _, c := range cases {

mockStdin := strings.NewReader(c.Input)

Copy link

Contributor

zlesnr Sep 21, 2020

Choose a reason for hiding this comment

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

This is neat.

nicolasjhampton changed the title ~~feat(release): Issue #45 WIP add support for command chaining~~ feat(release): Issue #45 add support for command chaining Sep 21, 2020

zlesnr merged commit ba0c521 into newrelic:master Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(release): Issue #45 add support for command chaining #368

feat(release): Issue #45 add support for command chaining #368

nicolasjhampton commented Aug 31, 2020 •

edited

Loading

CLAassistant commented Aug 31, 2020

zlesnr commented Aug 31, 2020

nicolasjhampton commented Aug 31, 2020 •

edited

Loading

nicolasjhampton commented Aug 31, 2020

zlesnr left a comment

zlesnr Sep 18, 2020

nicolasjhampton Sep 18, 2020

zlesnr Sep 18, 2020

nicolasjhampton Sep 18, 2020

zlesnr Sep 21, 2020

zlesnr Sep 18, 2020

nicolasjhampton Sep 18, 2020

zlesnr Sep 18, 2020

nicolasjhampton Sep 18, 2020

zlesnr left a comment

zlesnr Sep 21, 2020

zlesnr commented Sep 21, 2020


		var pipeInput map[string][]string = map[string][]string{}

		func Get(inputKey string) ([]string, bool) {


		var GetInput = getPipeInputFactory(stdinPipeReader{input: os.Stdin}, pipeInputExists)

		var pipeInput map[string][]string = map[string][]string{}

feat(release): Issue #45 add support for command chaining #368

feat(release): Issue #45 add support for command chaining #368

Conversation

nicolasjhampton commented Aug 31, 2020 • edited Loading

What

Next work needed

Documentation

CLAassistant commented Aug 31, 2020

zlesnr commented Aug 31, 2020

nicolasjhampton commented Aug 31, 2020 • edited Loading

nicolasjhampton commented Aug 31, 2020

zlesnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zlesnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zlesnr commented Sep 21, 2020

nicolasjhampton commented Aug 31, 2020 •

edited

Loading

nicolasjhampton commented Aug 31, 2020 •

edited

Loading