This repository has been archived by the owner on Jun 29, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 47
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
501db15
to
0b284c3
Compare
d67a2c0
to
a2cf558
Compare
johananl
commented
Aug 11, 2020
e45db75
to
e82451b
Compare
invidian
suggested changes
Aug 12, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR @johananl 👍
e82451b
to
7d33836
Compare
invidian
previously approved these changes
Aug 12, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though it is possible to do even more fair-grained commits, for example:
- pkg/dns: handle stdin read errors
- pkg/terraform: add OutputBytes function (to unexport
executeSync
) - pkg/dns: use terraform.OutputBytes instead of terraform.ExecuteSync
- pkg/terraform: add ExecutionStep concept
- pkg/dns: add ManualConfigPrompt function
- pkg/platform/packet: switch to use terraform.ExecutionStep
- pkg/terraform: unexport low-level functions
I added comments regarding some nits, but they are not very important for the scope of this PR.
- Define a new type terraform.ExecutionStep which allows describing arbitrary Terraform operations declaratively. - Allow passing one or more ExecutionSteps to the Terraform executor. This allows making pkg/platform generic as now every platform implementation can define its steps for deploying infrastructure in a declarative way, thus making the code which interacts with Terraform generic. Thanks to the PreExecutionHook concept, the same is true even when arbitrary logic needs to be executed before some Terraform operation: A platform implementation can include a callback function in the PreExecutionHook field of an ExecutionStep and the generic Terraform code will invoke it at the right time.
fmt.Scanln() returns an error which we should handle. fmt.Scanln() returns an error not only when there was a problem reading from stdin, but also when a newline is returned on its own, i.e. when the user hits Enter without typing "skip". Since we rely on the user hitting Enter for looping here, in order to distinguish between an actual error and the user hitting Enter, we would have to check for the sentinel value "unexpected newline" which isn't exported as a constant by the fmt package. By using ReadString() we can handle read errors simply without treating newlines as an error.
ExecuteSync() and ExecuteAsync() are low level functions and are used mainly internally by pkg/terraform. Add a dedicated Output() method to the Executor API. Callers should not have to allow on low-level functions to get outputs.
7d33836
to
e1f7508
Compare
invidian
approved these changes
Aug 12, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
surajssd
approved these changes
Aug 14, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 PR paves the way towards #716. By making
terraform.Executor
accept an arbitrary series of "steps" we can now declaratively define the infrastructure pieces a platform is composed of.Example:
I've deliberately avoided refactoring all the various platforms to keep the scope of this PR narrow. I've only refactored
platform/packet
(which is our most complex platform implementation at the moment) to demonstrate how the new structure can be used. The rest of the platforms will be refactored in subsequent PRs.