Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait improvements #161

Merged
merged 1 commit into from
Jul 11, 2021
Merged

Wait improvements #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions iterative/aws/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"net"
"regexp"
"sort"
"strconv"
Expand All @@ -16,8 +15,6 @@ import (
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/aws/aws-sdk-go/service/sts"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"

"terraform-provider-iterative/iterative/utils"
)

//ResourceMachineCreate creates AWS instance
Expand Down Expand Up @@ -440,7 +437,3 @@ func decodeAWSError(region string, err error) error {

return fmt.Errorf("Not able to decode: %s", err.Error())
}

func ResourceMachineLogs(ctx context.Context, d *schema.ResourceData, m interface{}) (string, error) {
return utils.RunCommand("journalctl --no-pager", 10*time.Second, net.JoinHostPort(d.Get("instance_ip").(string), "22"), "ubuntu", d.Get("ssh_private").(string))
}
7 changes: 0 additions & 7 deletions iterative/azure/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"net"
"os"
"strings"
"time"
Expand All @@ -17,8 +16,6 @@ import (
"github.com/Azure/go-autorest/autorest/to"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"

"terraform-provider-iterative/iterative/utils"
)

//ResourceMachineCreate creates AWS instance
Expand Down Expand Up @@ -396,7 +393,3 @@ func subscriptionID() (string, error) {

return "", errors.New("AZURE_SUBSCRIPTION_ID is not present")
}

func ResourceMachineLogs(ctx context.Context, d *schema.ResourceData, m interface{}) (string, error) {
return utils.RunCommand("journalctl --no-pager", 10*time.Second, net.JoinHostPort(d.Get("instance_ip").(string), "22"), "ubuntu", d.Get("ssh_private").(string))
}
11 changes: 6 additions & 5 deletions iterative/resource_machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/base64"
"fmt"
"net"
"time"

"terraform-provider-iterative/iterative/aws"
Expand Down Expand Up @@ -236,13 +237,13 @@ func resourceMachineRead(ctx context.Context, d *schema.ResourceData, m interfac

func resourceMachineLogs(ctx context.Context, d *schema.ResourceData, m interface{}) (string, error) {
switch cloud := d.Get("cloud").(string); cloud {
case "aws":
return aws.ResourceMachineLogs(ctx, d, m)
case "azure":
return azure.ResourceMachineLogs(ctx, d, m)
case "kubernetes":
return kubernetes.ResourceMachineLogs(ctx, d, m)
default:
return "", fmt.Errorf("Unknown cloud: %s", cloud)
return utils.RunCommand("journalctl --no-pager",
2*time.Second,
net.JoinHostPort(d.Get("instance_ip").(string), "22"),
"ubuntu",
d.Get("ssh_private").(string))
}
}
13 changes: 12 additions & 1 deletion iterative/resource_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"encoding/json"
"fmt"
"log"
"net"
"os"
"strconv"
"text/template"
Expand Down Expand Up @@ -210,7 +211,17 @@ func resourceRunnerCreate(ctx context.Context, d *schema.ResourceData, m interfa
var logError error
var logEvents string
err = resource.Retry(d.Timeout(schema.TimeoutCreate), func() *resource.RetryError {
logEvents, logError = resourceMachineLogs(ctx, d, m)
switch cloud := d.Get("cloud").(string); cloud {
case "kubernetes":
logEvents, logError = resourceMachineLogs(ctx, d, m)
default:
logEvents, logError = utils.RunCommand("journalctl --unit cml --no-pager",
2*time.Second,
net.JoinHostPort(d.Get("instance_ip").(string), "22"),
"ubuntu",
d.Get("ssh_private").(string))
}

Comment on lines +214 to +224
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to keep a separate resourceMachineLogs function for each provider, even if some of them share the same code. The proposed changes are introducing two additional calls for non-SSH vendors, and it doesn't feel quite elegant, in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO having duplicate code is not elegant at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And of course we are having the resource machine which can have intermediate business logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proposed changes are introducing two additional calls for non-SSH vendors

Which two calls?

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO having duplicate code is not elegant at all

Agreed! Nevertheless, in this case we're just duplicating a line of code for the sake of having the same methods on every provider. We aren't copying and pasting a copy of the utils.RunCommand function, but just a single line call. 🙃 The current pattern would be much more clean after #147.

And of course we are having the resource machine which can have intermediate business logic

Intermediate busniess logic is a pretty general term here. While 66% of our supported vendors — or 75% very soon — are going to rely on SSH for log gathering, that percentage will considerably decrease as soon as we start adding more container orchestrators. Should we count a specific log gathering mechanism as intemediate business logic just because it's the same for a couple of providers?

Copy link
Member

Choose a reason for hiding this comment

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

Which two calls?

For orchestrators and other non-SSH providers, we would be calling the following two blocks sequentially, hitting two identical switch statements (hello code duplication) and making two calls: one to resourceMachineLogs and other to provider.ResourceMachineLogs

terraform-provider-iterative/iterative/resource_runner.go

Lines 214 to 223 in 05aa3a3

switch cloud := d.Get("cloud").(string); cloud {
case "kubernetes":
logEvents, logError = resourceMachineLogs(ctx, d, m)
default:
logEvents, logError = utils.RunCommand("journalctl --unit cml --no-pager",
2*time.Second,
net.JoinHostPort(d.Get("instance_ip").(string), "22"),
"ubuntu",
d.Get("ssh_private").(string))
}

terraform-provider-iterative/iterative/resource_machine.go

Lines 238 to 249 in 05aa3a3

func resourceMachineLogs(ctx context.Context, d *schema.ResourceData, m interface{}) (string, error) {
switch cloud := d.Get("cloud").(string); cloud {
case "kubernetes":
return kubernetes.ResourceMachineLogs(ctx, d, m)
default:
return utils.RunCommand("journalctl --no-pager",
2*time.Second,
net.JoinHostPort(d.Get("instance_ip").(string), "22"),
"ubuntu",
d.Get("ssh_private").(string))
}
}

log.Printf("[DEBUG] Collected log events: %#v", logEvents)
log.Printf("[DEBUG] Connection errors: %#v", logError)

Expand Down