OC Migrate Blocks to using AttributeHandlers #269
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
June 23, 2020 14:14
…d). No Read function for ElasticSearch and FTP to align with master
added 17 commits
June 23, 2020 14:18
phamann
requested changes
Jun 23, 2020
There was a problem hiding this comment.
This is looking great! Really happy with how it's turned out. My review comments are very minor relating to comment/documentation style. @mccurdyc has picked up most of the code issues so I'll leave those to his review.
- Very pedantic but is Wasm not WASM. Per: https://webassembly.org/
- Slightly pedantic, per the golang code review guidelines we prefer comments to be full sentences including ending with a period. I know a lot of this code base doesn't follow this, but I try to pick it up where-ever possible on function comments, inline ones can be left as-is.
- Some places I've asked for comments to be added to help document the pattern more.
- Lastly, It would be great if you can add a detailed description to this PR for future maintainers, if they stubble upon the large changeset and want to understand the reasoning why we went with this pattern.
fastly/base_fastly_service_v1.go
Outdated
Comment on lines
282
to
286
| // New versions are not immediately found in the API, or are not | ||
| // immediately mutable, so we need to sleep a few and let Fastly ready | ||
| // itself. Typically, 7 seconds is enough | ||
| log.Print("[DEBUG] Sleeping 7 seconds to allow Fastly Version to be available") | ||
| time.Sleep(7 * time.Second) |
There was a problem hiding this comment.
@thommahoney not blocking for this PR, but I'd like to understand why this is the case.
mccurdyc
approved these changes
Jun 23, 2020
| @@ -208,3 +94,7 @@ func TestEscapePercentSign(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func appendNewLine(s string) string { | |||
There was a problem hiding this comment.
Not blocking. I think this may be dead now.
| // MustProcess returns whether we must process the resource (usually HasChange==true but allowing exceptions). | ||
| // For example: at present, the settings attributeHandler (block_fastly_service_v1_settings.go) must process when | ||
| // default_ttl==0 and it is the initialVersion - as well as when default_ttl or default_host have changed. | ||
| MustProcess(d *schema.ResourceData, initialVersion bool) bool |
There was a problem hiding this comment.
Just noting that we talked offline about how it is okay that we just use initialVersion here for now rather than solving the more general use of this method.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a large change which refactors how attributes are processed for service resources. The change is motivated primarily to enable composition of different attribute sets for services of type Wasm and VCL; secondarily to tidy up the codebase and refactor large files.
AttributesHandlers provide contributions to the resource schema (Register), resource processing (Process) and resource state refresh (Read). These functions are delegated to AttributeHandlers by a new Base codebase shared between Wasm and VCL service resources.