Adds support for managing edge dictionaries #159

benzvan · 2020-09-28T15:42:42Z

This is addressing issue #153

I'm hoping someone can take a look at this. The test will fail because the help function isn't printing what is expected. I still don't quite understand the code well enough to debug this. The edgedictionary/describe.go code is nearly identical to the healthcheck/describe.go code and the root.go files are also basically the same. But somehow the output of the help is different between the two.

benzvan · 2020-09-28T15:52:41Z

Never mind. Found my problem.

benzvan · 2020-09-28T16:58:09Z

I have plans to add functions for create, update, and delete as well. I can either add them to this PR or I can wait until this is merged and add them individually, whichever makes sense for the flow of this project.

benzvan · 2020-09-28T20:29:00Z

After some consideration, I decided describe wasn't complete enough for a version update and continued working. This is still work in progress and only has describe and create at this point.

benzvan · 2020-10-02T17:07:21Z

I believe this to be a releasable update with minimum viable features for dictionaries and dictionary items. I apologize for the size of this change, but each commit is relatively self-contained for easier reviewing.

My one concern is that if one wanted to create a new dictionary item one would need to do the following:

fastly service search --name <servicename>
get the service ID and service version from the above command
fastly service-version clone ...
get the service version from the above command
fastly dictionary create ...
get the dictionary ID from the above command
fastly service-version activate ...
fastly dictionaryitem create ...

Similarly updating an existing dictionary item will require describing the dictionary to get the dictionary id first.

I would propose SERVICE-ID be an optional flag provided SERVICE-NAME is present and can be used to query fastly for the service ID. Same with DICTIONARY-ID and DICTIONARY-NAME.

I would also propose a flag like clone-service=true for dictionary create that would combine the first 6 steps listed above so that dictionaries can be created and applied to a service in a single step.

benzvan · 2020-10-13T14:49:38Z

Just checking if anyone has had a chance to look at this. @phamann ?

phamann

@benzvan apologies for the delay here, I appreciate both your effort for the contribution and the poke, so thank you.

Overall, this is looking good. A couple of small points around UX and consistency with the rest of the project. But nothing major.

imilarly updating an existing dictionary item will require describing the dictionary to get the dictionary id first.

I would propose SERVICE-ID be an optional flag provided SERVICE-NAME is present and can be used to query fastly for the service ID. Same with DICTIONARY-ID and DICTIONARY-NAME.

This is a good feature request. If you don't want to add the functionality for dictionaries in this PR please feel free to raise this as an issue on the repo and we'll try and get it added to the roadmap.

I would also propose a flag like clone-service=true for dictionary create that would combine the first 6 steps listed above so that dictionaries can be created and applied to a service in a single step.

Again this is a great feature request, so fell free to add an issue for tracking.

pkg/edgedictionaryitem/create.go

pkg/edgedictionary/describe.go

pkg/edgedictionaryitem/root.go

pkg/edgedictionaryitem/create.go

benzvan · 2020-10-13T18:00:37Z

Thanks for the feedback. I'll get to work on that and I'll add an issue for the feature enhancement.

benzvan · 2020-10-13T20:15:02Z

I've addressed your comments with new commits and one comment.

I'm willing to add both of the features I mention above within this PR but I'm having trouble figuring out if kingpin supports either/or required flags or if that's something I'll need to add to each file.

benzvan · 2020-10-15T16:40:41Z

I'm going to pause for a moment (assuming tests pass) to allow you to look at my latest two commits. I'd like to make sure I'm matching your expectations from a project structure standpoint. I created a new service.go file that contains the GetIDFromName() function that is used by both the service describe and dictionary create commands.

benzvan · 2020-10-21T16:08:41Z

@phamann just poking you for input again. I'm not sure I'll have time to do more on this for another week or so.

phamann · 2020-10-29T14:16:25Z

Hey @benzvan apologies for the delay on this PR. We were focused on getting the AssemblyScript support out first #160.

With regards two your last two commits, if not too much pain for you, I'd prefer it if you can move the --service-name work to its own PR, as its an important feature that warrants being in the change log, which is generated on a per PR basis as its automated. It will also allow us to review it in isolation. Hope thats ok? I should have said this earlier, apologies.

Once thats out, I think this PR should be good to go. 😄

benzvan · 2020-10-29T15:51:58Z

Not a problem. I'll pull those commits and start a new branch.

…

On Thu, Oct 29, 2020, 09:16 Patrick Hamann ***@***.***> wrote: Hey @benzvan <https://github.com/benzvan> apologies for the delay on this PR. We were focused on getting the AssemblyScript support out first #160 <#160>. With regards two your last two commits, if not too much pain for you, I'd prefer it if you can move the --service-name work to its own PR, as its an important feature that warrants being in the change log, which is generated on a per PR basis as its automated. It will also allow us to review it in isolation. Hope thats ok? I should have said this earlier, apologies. Once thats out, I think this PR should be good to go. 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#159 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWP6RYVTIEZBNA7HFGONPTSNF2MTANCNFSM4R4VLOQA> .

benzvan · 2020-11-02T18:41:18Z

OK. I force-pushed to my branch without the last two commits and it should be ready to merge.

benzvan · 2020-11-02T18:41:38Z

Oh...except I need to pull from master...1 sec.

benzvan · 2020-11-27T19:49:18Z

This is ready for another review.

benzvan · 2020-12-08T16:20:53Z

Just thought I'd bump this @phamann @Integralist

Integralist · 2020-12-08T16:25:27Z

👋🏻 @benzvan

I'm now back from paternity leave! 🥳

And yes, I've got this PR scheduled in my calendar to be re-reviewed and for which I aim to start tomorrow morning (UK time), so hopefully we can get this over the finish line very soon! 🙂

configure_test.go and edgedictionaryitem_test.go both had a need for a tempfile, so I simplified the method from configure_test and added it to the testutils where other common test utils go.

benzvan · 2020-12-08T18:01:58Z

Congrats on your new parentage @Integralist !

Thanks for the note. I just wanted to make sure it hadn't been forgotten. :-D

Integralist · 2020-12-09T13:39:45Z

Just a heads up that it seems the Windows test run has failed because of a difference in error message...

want "open missingFile: no such file or directory",
have "open missingFile: The system cannot find the file specified."

Which is interesting, as it's an OS specific error 🤔

Here is the full error:

--- FAIL: TestDictionaryItemBatchModify (0.08s)
    --- FAIL: TestDictionaryItemBatchModify/dictionaryitem_batchmodify_--service-id_123_--dictionary-id_456_--file_missingFile (0.01s)
        edgedictionaryitem_test.go:299: want "open missingFile: no such file or directory", have "open missingFile: The system cannot find the file specified."

Integralist · 2020-12-09T13:57:12Z

re:

The example request json for the bulk update dictionary item endpoint is invalid.

Thanks for notifying me of this! I've got a PR to correct that now 👍🏻

benzvan · 2020-12-09T14:42:25Z

That's really odd...I guess go is passing the system error back up as an error, which is going to be hard to test given the system error is different depending on the system. Maybe I can catch the error type and replace it with a consistent message.

Windows tests were failing due to a difference in the text content of the missingFile error. I reduced the error expectation to the portion that I know is not OS dependent.

Integralist · 2020-12-09T15:00:39Z

@benzvan didn't want you to miss this comment: #159 (comment)

Integralist · 2020-12-09T16:30:30Z

@benzvan re:

So I'm still using Flag.Required() to make sure the --new-name flag was passed but then also using the Action function to call c.newname.Set.

So this might just be a simple misunderstanding regarding the Dictionary update command. Let me try and explain...

If I look at the Go-Fastly code for updating a dictionary I see the following...

// UpdateDictionaryInput is used as input to the UpdateDictionary function.
type UpdateDictionaryInput struct {
	// ServiceID is the ID of the service (required).
	ServiceID string

	// ServiceVersion is the specific configuration version (required).
	ServiceVersion int

	// Name is the name of the dictionary to update.
	Name string

	NewName   *string      `form:"name,omitempty"`
	WriteOnly *Compatibool `form:"write_only,omitempty"`
}

This tells me that both the 'new name' and the 'write only' fields are optional and thus using Required() in the CLI for the --new-name flag isn't actually necessary in this case, because strictly speaking they should be able to specify a --write-only flag by itself (or both --write-only and --new-name together).

benzvan · 2020-12-09T17:11:38Z

So this might just be a simple misunderstanding regarding the Dictionary update command. Let me try and explain...

OK, this makes sense. I didn't implement the --write-only option in the CLI, so --new-name is required for it to do anything.

Integralist

Thanks @benzvan for your hard work. I've left a few more comments. But it looks like we're getting very close now 🤞🏻

pkg/edgedictionary/update.go

pkg/edgedictionaryitem/batch.go

pkg/edgedictionaryitem/create.go

pkg/edgedictionaryitem/describe.go

pkg/edgedictionaryitem/list.go

pkg/edgedictionaryitem/describe.go

pkg/edgedictionaryitem/update.go

Integralist · 2020-12-09T17:51:35Z

pkg/edgedictionaryitem/update.go

+	c.CmdClause.Flag("service-id", "Service ID").Short('s').StringVar(&c.manifest.Flag.ServiceID)
+	c.CmdClause.Flag("dictionary-id", "Dictionary ID").Required().StringVar(&c.Input.DictionaryID)
+	c.CmdClause.Flag("key", "Dictionary item key").Required().StringVar(&c.Input.ItemKey)
+	c.CmdClause.Flag("value", "Dictionary item value").Required().Action(c.itemvalue.Set).StringVar(&c.itemvalue.Value)


So annoyingly the Go-Fastly implementation has the item 'value' as an optional field although I believe that to be a mistake and in fact it should be a required field (because why bother calling the api without an updated value for the key -- I don't believe it's even possible to set an empty string as a value for an edge dict key either).

For now let's keep the .Required() as we want the command to be useful (i.e. don't allow someone to make request without a value) but we'll still need the common.OptionalString for now as the version of Go-Fastly we have is internally using a pointer. When I eventually cut a new Go-Fastly release I'll come back (as per the TODO(integralist) comment above) and fix up this section.

pkg/text/dictionaryitem_test.go

The `dictionary describe --verbose` command makes additional calls to the fastly api's GetDictionaryInfo and ListDictionaryItems endpoints. The call to GetDictionaryInfo was using the serviceID value in place of the dictionaryID, which would have resulted in a failed call at best and values from the wrong dictionary at worst. I added enforcement in the mock call to that endpoint to verify that the dictionaryID in the input is the same as the dictionaryID in the output from the original call to GetDictionary.

Add TODO for cleanup due to errors in the current go-fastly release. Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>

benzvan · 2020-12-09T22:13:26Z

@Integralist I'm reasonably certain I addressed all your comments.

Integralist

@benzvan please don't hate me 🙂 but there's three tiny textual tweaks I'd like to make.

Once you've got those made I'll be able to merge and then cut a new release 🎉

pkg/edgedictionaryitem/create.go

pkg/edgedictionary/edgedictionary_test.go

pkg/edgedictionary/update.go

Small text changes that would have bothered me for ages once I noticed them. Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>

benzvan · 2020-12-11T14:15:37Z

@Integralist Not at all. Except for the remediation message, which I wasn't sure how to word, the other two would have bugged me once I noticed them.

Integralist

@benzvan I think we've made it over the finish line 🎉

Thank you for your patience and hard work on this PR Ben ❤️

I took back over review of this PR last week.

Integralist · 2020-12-14T11:19:11Z

@benzvan v0.21.0 CLI release can be found here.

benzvan changed the title ~~WIP: Adding support for dictionary describe~~ Adding support for dictionary describe Sep 28, 2020

benzvan force-pushed the master branch from 568d87c to 448e7b9 Compare September 28, 2020 17:46

benzvan force-pushed the master branch 3 times, most recently from 954c37e to 103e83e Compare October 2, 2020 14:22

benzvan changed the title ~~Adding support for dictionary describe~~ Adds support for managing edge dictionaries Oct 6, 2020

phamann requested changes Oct 13, 2020

View reviewed changes

pkg/edgedictionaryitem/create.go Outdated Show resolved Hide resolved

pkg/edgedictionary/describe.go Outdated Show resolved Hide resolved

pkg/edgedictionaryitem/root.go Outdated Show resolved Hide resolved

pkg/edgedictionaryitem/create.go Outdated Show resolved Hide resolved

benzvan force-pushed the master branch 2 times, most recently from 556eeab to e6e7938 Compare October 15, 2020 16:39

benzvan force-pushed the master branch from e6e7938 to 82f0fd1 Compare November 2, 2020 18:38

benzvan added 7 commits November 2, 2020 12:42

Adds dictionary describe

2ed6818

adds dictionary create

e7d0a51

adds dictionary delete

5506ea3

adds dictionaryitem describe

ba872a0

adds dictionaryitem list

ea981b3

adds dictionaryitem create

24918bc

adds dictionaryitem update

404b7f3

benzvan force-pushed the master branch from 590cae0 to bdd4ee4 Compare November 27, 2020 19:00

Combines multiple uses of MakeTestFile into testutils

765627d

configure_test.go and edgedictionaryitem_test.go both had a need for a tempfile, so I simplified the method from configure_test and added it to the testutils where other common test utils go.

Reduce specificity of wantError for missing file

2ceba45

Windows tests were failing due to a difference in the text content of the missingFile error. I reduced the error expectation to the portion that I know is not OS dependent.

Integralist reviewed Dec 9, 2020

View reviewed changes

benzvan and others added 7 commits December 9, 2020 12:07

deletes prefix property from dictionaryitem_test

095f994

fix dictionary id/name in dictionaryitem describe

abf3f87

fix copy/paste 'describe a service' in new doc comments

5852351

adds write-only option to dictionary create command

7fccc4e

adds write-only option to dictionary update command

94dc8c4

Update pkg/edgedictionaryitem/update.go

d7649dc

Add TODO for cleanup due to errors in the current go-fastly release. Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>

benzvan force-pushed the master branch from b3a5d14 to d7649dc Compare December 9, 2020 22:07

Integralist reviewed Dec 11, 2020

View reviewed changes

pkg/edgedictionaryitem/create.go Outdated Show resolved Hide resolved

pkg/edgedictionary/edgedictionary_test.go Outdated Show resolved Hide resolved

pkg/edgedictionary/update.go Outdated Show resolved Hide resolved

Apply suggestions from code review

a4309e7

Small text changes that would have bothered me for ages once I noticed them. Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>

Integralist approved these changes Dec 14, 2020

View reviewed changes

Integralist merged commit 0f4d27d into fastly:master Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for managing edge dictionaries #159

Adds support for managing edge dictionaries #159

benzvan commented Sep 28, 2020 •

edited

Loading

benzvan commented Sep 28, 2020

benzvan commented Sep 28, 2020

benzvan commented Sep 28, 2020

benzvan commented Oct 2, 2020

benzvan commented Oct 13, 2020

phamann left a comment

benzvan commented Oct 13, 2020

benzvan commented Oct 13, 2020

benzvan commented Oct 15, 2020 •

edited

Loading

benzvan commented Oct 21, 2020

phamann commented Oct 29, 2020

benzvan commented Oct 29, 2020 via email

benzvan commented Nov 2, 2020

benzvan commented Nov 2, 2020

benzvan commented Nov 27, 2020

benzvan commented Dec 8, 2020

Integralist commented Dec 8, 2020

benzvan commented Dec 8, 2020

Integralist commented Dec 9, 2020

Integralist commented Dec 9, 2020

benzvan commented Dec 9, 2020

Integralist commented Dec 9, 2020

Integralist commented Dec 9, 2020

benzvan commented Dec 9, 2020

Integralist left a comment

Integralist Dec 9, 2020

benzvan commented Dec 9, 2020

Integralist left a comment

benzvan commented Dec 11, 2020

Integralist left a comment

Integralist commented Dec 14, 2020

Adds support for managing edge dictionaries #159

Adds support for managing edge dictionaries #159

Conversation

benzvan commented Sep 28, 2020 • edited Loading

benzvan commented Sep 28, 2020

benzvan commented Sep 28, 2020

benzvan commented Sep 28, 2020

benzvan commented Oct 2, 2020

benzvan commented Oct 13, 2020

phamann left a comment

Choose a reason for hiding this comment

benzvan commented Oct 13, 2020

benzvan commented Oct 13, 2020

benzvan commented Oct 15, 2020 • edited Loading

benzvan commented Oct 21, 2020

phamann commented Oct 29, 2020

benzvan commented Oct 29, 2020 via email

benzvan commented Nov 2, 2020

benzvan commented Nov 2, 2020

benzvan commented Nov 27, 2020

benzvan commented Dec 8, 2020

Integralist commented Dec 8, 2020

benzvan commented Dec 8, 2020

Integralist commented Dec 9, 2020

Integralist commented Dec 9, 2020

benzvan commented Dec 9, 2020

Integralist commented Dec 9, 2020

Integralist commented Dec 9, 2020

benzvan commented Dec 9, 2020

Integralist left a comment

Choose a reason for hiding this comment

Integralist Dec 9, 2020

Choose a reason for hiding this comment

benzvan commented Dec 9, 2020

Integralist left a comment

Choose a reason for hiding this comment

benzvan commented Dec 11, 2020

Integralist left a comment

Choose a reason for hiding this comment

Integralist commented Dec 14, 2020

benzvan commented Sep 28, 2020 •

edited

Loading

benzvan commented Oct 15, 2020 •

edited

Loading