v2

[Integralist]

Problem: v1 was lacking critical support for WAF APIs.
Solution: implement missing APIs + fix some minor issues (see notes below) and bump to v2.
Notes: We now...

• Use consistent naming (e.g ServiceID, ServiceVersion, DictionaryID, PoolID).
• Use pointers as field values for optional basic types (reference material).
• Implement the /stats/field/... API endpoint (as per Fails to Parse Historic Stats when no Service Provided #214).

[Integralist]

@phamann as you're aware, the WAF aspect of this PR has been reviewed and approved already, it's the last three commits authored by myself that require a solid review.

I appreciate that most of the code is boilerplate in appearance (and will be tedious to sift through) but it's these types of changes that make me especially nervous, so I'm going to be going back over it myself with a fine tooth-comb and would appreciate you do the same if you have capacity.

The section I'm most concerned about is the move to pointer values for optional basic types. As an example, there were some situations where a basic type was optional and yet I didn't switch it to be a pointer simply because the basic type was wrapped in a custom type that was defined with constant values (i.e. in theory there should always be a value provided by the user as they would have to assign one of the custom type's constant values to the field).

Similarly there were types such as []string where we had discussed previously (with @mccurdyc) that maybe this shouldn't need to become []*string but I can't now recall the rationale behind that decision.

[phamann]

Firstly, thank you SO MUCH for taking the time to do this, I know how monotonous it may feel. ❤️

I've taken a scan through the changes and no major issues stood out or typos etc. However, I have are some general observations/questions/discussions to start:

Use of pointers:
I noticed that you've also implemented the pointer pattern on the CreateFooInput create structs. However, as I understand, they're not needed for resource creation as the underlying omit empty issue only relates to updating fields on existing resources and not creating new ones. Did you do this for consistency or another reason I’m not thinking of? What is your opinion on using them?

ServiceID and ServiceVersion consistency:
I noticed that you've made these consistent on all of the input structs but not on the response models themselves (i.e. Backend vs CreateBackendInput). Is there a reason for this? I assume to be consistent with the API schema returned in the JSON responses? However it does raise the question of whether we should be consistent in all structs. Which I'd personally say we should.

Go module v2:
Something we haven't discussed internally yet is how we are going to release this properly as v2 and place nicely with go modules.

Sadly, like most things with Go dependency management, the official recommendation and how other repos approach this are a bit conflicting. Golang recommends creating a v2 folder in the master/main branch and continue the v2 development in there, see: https://blog.golang.org/v2-go-modules and https://github.com/googleapis/gax-go. Mostly, this recommendation is based on keeping the repo working for tools that do not support go modules yet.

Hashicorp has used a different approach with branches. They have a long-lived v2 branch (i.e hcl2) and the go.mod in that branch has a package called module xxxx/v2 (ie. module github.com/hashicorp/hcl/v2). See https://github.com/hashicorp/hcl/tree/hcl2. We took this approach in the interim for WAF and TF while we were still developing v2. Hence the current dev-v2 branch in this PR. As it allowed us to start consuming it via go modules in TF but not advertise to external users there is an official v2 release yet.

So we have a decision to make:

• Move all of this code to a v2 directory
• Keep a v2 branch as mainline
• Merge this as-is and become v2 in master

Upgrading docs:
Related to the above discussion, once we decide upon how we are going to version and releases as a v2 go module, we need to document how users can upgrade from v1 to v2 and enumerate the main changes to watch out for.

[phamann]

Does this need to be a pointer?

[Integralist]

Thanks @phamann

1. re: pointers - I'm going to do as you've suggested and only implement the pointer references for input structs that are used for write/update operations.

2. re: response object consistency - good point. I'll make those consistent as well.

3. re: versioning - I'm not a fan of having to deal with two code bases separated by just a folder (makes things confusing when editing files as I could imagine finding myself accidentally updating the wrong file, e.g. a v1 vs v2 file of the same name when fuzzing searching).

I'm also not sure what the repercussions are with regards to the simpler v2 suffix on the go.mod namespace? e.g. we rename the module namespace and merge this branch into master. I know that the consumer of Go-Fastly would need to opt in by updating their imports to reference the v2 but then if they decide they're not ready to implement that change, then what happens? e.g. consumers can't continue to import the old go.mod value of github.com/fastly/go-fastly as that won't exist in master any more. But then I believe that's only an issue if we have customers expecting v1 to be maintained (e.g. what about security updates?) because otherwise the consumer's go.mod dependency wouldn't need to change (unless go's tooling for modules discovers the latest commit and tries to use it, then that would break their expectations and then they'd have to hand craft their go.mod to reference a v1.0.0 tag or specific pre-v2 commit).

So this might suggest that keeping the dev-v2 branch (and the go.mod change to github.com/fastly/go-fastly/v2) but renaming it to v2 and then leaving this code here as being the better option. My concern with that is the discoverability of it. We'd need to manually update the README for the master branch to state we have a v2 branch available as I don't believe the go tooling supports discovering these changes currently (I'm guessing we'd still need to tag the latest commit to be v2.0.0, or can a go.mod reference a branch name?).

Alternatively could we do it in reverse? Could we create a v1 branch off of current master (tag it as v1.0.0 if that's not already done), then merge our dev-v2 into master? I'd imagine we'd still need to update the go.mod namespace to be github.com/fastly/go-fastly/v2. But at least that way a consumer of this library can get v2 by default on a new project, while still able to easily reach v1 if necessary for an existing consumer.

[phamann]

🎉 LGTM - SHIP IT!