Cli tool #2806

alexsomesan · 2018-01-23T15:11:55Z

Same as original, but correctly rebased.

coreosbot · 2018-01-23T15:11:57Z

Can one of the admins verify this patch?

squat

There are a few changes I'd like to see. Overall, the code probably works for most uses but it needs some cleanup

squat · 2018-01-23T15:26:24Z

BUILD.bazel

 )

-exports_files(["config.tf"])
+PLATFORMS = ["aws", "azure", "gcp", "govcloud", "metal", "openstack/neutron", "vmware"]


I think something funny happened with the rebase: these definitions are out of date with ut2-integration. We no longer define PLATFORMS in BUILD.bazel

squat · 2018-01-23T15:26:49Z

BUILD.bazel

-exports_files(["config.tf"])
+PLATFORMS = ["aws", "azure", "gcp", "govcloud", "metal", "openstack/neutron", "vmware"]
+
+[genrule(


Same here, these rules are no defined in examples/BUILD.bazel and Documentation/BUILD.bazel

squat · 2018-01-23T15:27:17Z

installer/cmd/tectonic/BUILD.bazel

+    ],
+)
+
+go_binary(


we should add explicit rules for darwin and linux. can be done in a followup but it is a requirement before the task is done.

squat · 2018-01-23T15:27:40Z

installer/cmd/tectonic/main.go

+			buildPath := *deleteClusterDir
+			pathStat, err := os.Stat(buildPath)
+			// TODO: add deeper checking of the path for having cluster state
+			if os.IsNotExist(err) || !pathStat.IsDir() {


what about any other syscall-related errors? is this error actually safe to ignore?

squat · 2018-01-23T15:28:09Z

installer/pkg/tectonic/buildstate.go

+	scanner := bufio.NewScanner(vf)
+	for scanner.Scan() {
+		line := scanner.Text()
+		if strings.HasPrefix(line, "tectonic_cluster_name") {


tectonic_cluster_name should be made into a constant

This whole method is only meant to be around until we merge Daniel's config object parser. Then we scrap this and get the name from the config object.
I agree though, technically "tectonic_cluster_name" should be a constant. I also expect the compiler would optimize towards that, no?

Im suggesting it from code maintainability perspective.

You've got a valid point. But this method is disposable and should only be in here until Daniel gets to merge his stuff in. I'll add the constant thought.

squat · 2018-01-23T15:33:49Z

installer/pkg/workflow/workflow.go

+	Execute() error
+}
+
+type Metadata map[string]interface{}


likewise, these types are implementation details of the Workflowtype type and should not be exported.

Agree. Making it unexported.

squat · 2018-01-23T15:34:02Z

installer/pkg/workflow/workflow.go

+
+type Metadata map[string]interface{}
+
+type Step interface {


Rather than model Step as an interface, why not model it as a function type? This way defining the list of steps in install.go would be easier and you wouldn't need to define throwaway structs just to define methods.

Simple steps appear as overkill with this approach I know, but I can imagine more complex ones would be split across multiple functions that would want to share some state. Even then we could explicitly pass around all necessary state as arguments (functional style) and still use a function type as entrypoint.
I tend to think this form is easier to reason with for most people so I'm tempted to keep it. If you feel strongly the other way, we can switch though.
Either way, empty structs should get optimised away by the compiler (unless they are being very "original") so they should not bloat in any way.

Agreed on the compiler angle. However, in order to avoid premature optimization for a potential future complexity, I would have gone with the function style first. It's possible we may need to make the change in the future, though it's also possible that if we go down the current route, we will never need the structs to carry any state and we'll have struct steps simply because we thought we'd need the complexity. Not a big problem either way. Again, not concerned about the binary performance but more about the legibility and maintainability

squat · 2018-01-23T15:34:15Z

installer/pkg/workflow/workflow.go

+}
+
+// Context represents the state of a workflow
+type WorkflowType struct {


This type should strictly be unexported, otherwise there is no real point to exporting the interface. If the Workflow interface is the API for all other packages, then this type should be internal, i.e. an implementation that is of no concern to users.

Please rename this type; it is odd in Go to suffix any type with Type just as we do not suffix interfaces with IFace or the like. If this is truly going to be one of several implementations then I suggest that this type be named after that implementation. On the other hand, if this type is going to be the only implementation, then there should be no interface at all and this should be the only exported type from this package. In this case this type should be named simply Workflow.

I had the same dilema myself.
The reality is we (or I) don't know for sure if this is going to suffice as a sole implementation or not in the long run. That's why the interface is there and it's being referenced via the interface. I think this is the safer design choice at this point.

How about we call this implementation SimpleWorkflow for now? I'm not great with picking good names..

sure call it something like basicWorkflow/simpleWorkflow but the type should really be unexported

squat · 2018-01-23T15:34:36Z

installer/pkg/workflow/workflow.go

+}
+
+func (w WorkflowType) Execute() error {
+	var stepError error


why not just name this err?

Old habit from other more talkative languages. I'll make it 'err'.

squat · 2018-01-23T15:34:48Z

installer/pkg/workflow/workflow.go

+
+func (w WorkflowType) Execute() error {
+	var stepError error
+	for _, oneStep := range w.steps {


it's convention in Go to name the range value either the singular of the splice, i.e. step, or the first letter, i.e. s.

Will change them.

alexsomesan · 2018-01-23T16:54:58Z

@squat Can you have a second look?
Made the changes you suggested (minus the ones to parts soon to be made obsolete by @spangenberg ).

squat · 2018-01-23T17:01:11Z

Looks like there may be some unnecessary commit in there. Can we rebase and drop dee61c5? that was never merged into any branch.

squat · 2018-01-23T17:05:49Z

installer/cmd/tectonic/main.go

+
+var (
+	dryRunFlag            = kingpin.Flag("dry-run", "Just pretend, but don't do anything.").Bool()
+	clusterInstallCommand = kingpin.Command("install", "Create a new Tectonic cluster.")


nit: the descriptions should all end without a period or all end with a period. I prefer no period but either way is ok as long as it is consistent

good point, I'll drop the periods.

squat · 2018-01-23T17:06:20Z

installer/cmd/tectonic/main.go

+			w := workflow.NewDestroyWorkflow(*deleteClusterDir)
+			w.Execute()
+		}
+	}


can we have a default case that shows the help?

Not sure I understand this right, but the Kingpin library actually produces automatic help based on those descriptions strings of each command. Here's how it looks today:

bazel-bin/installer/cmd/tectonic/darwin_amd64_stripped/tectonic --help usage: tectonic [<flags>] <command> [<args> ...] Flags: --help Show context-sensitive help (also try --help-long and --help-man). --dry-run Just pretend, but don't do anything. Commands: help [<command>...] Show help. install --config=CONFIG Create a new Tectonic cluster. delete [<dir>] Delete an existing Tectonic cluster.

We could obviously add additional information to it, but I presume this is enough to get started.

what happens if just execute tectonic? Will it show the help as well?

The same help output is shown.
This is all autogenerated by the kingpin CLI framework.

$ bazel-bin/installer/cmd/tectonic/darwin_amd64_stripped/tectonic usage: tectonic [<flags>] <command> [<args> ...] Flags: --help Show context-sensitive help (also try --help-long and --help-man). --dry-run Just pretend, but don't do anything. Commands: help [<command>...] Show help. install --config=CONFIG Create a new Tectonic cluster. delete [<dir>] Delete an existing Tectonic cluster.

squat · 2018-01-23T17:12:52Z

installer/pkg/workflow/workflow.go

+	return nil
+}
+
+func (m *metadata) SetValue(key string, value interface{}) error {


These methods should be unexported as well. Also, why are we wrapping the map with setters and getters?

Two reasons I had in mind when I did this.

The practical one is avoid the ugly syntax of dereferencing the m pointer every time since this is going to be done in a lot of places. May be a personal bias though.

The other one is a bit more on the hypothetical side: make sure we don't have to sweep through the whole code if we want to swap the map out for something else.

You think we should switch to accessing the map directly?

cpanato · 2018-01-30T11:18:42Z

installer/cmd/tectonic/main.go

+	clusterInstallCommand = kingpin.Command("install", "Create a new Tectonic cluster.")
+	clusterDeleteCommand  = kingpin.Command("delete", "Delete an existing Tectonic cluster.")
+	deleteClusterDir      = clusterDeleteCommand.Arg("dir", "The name of the cluster to delete").String()
+	clusterConfigFlag     = clusterInstallCommand.Flag("config", "Cluster specification file").Required().ExistingFile()


add a . at the end of the description. like the other 😄

enxebre · 2018-02-01T15:36:56Z

Just for having an starting point to discuss once this gets in, it will need to be integrated with #2802 so new organic abstractions will arise and in particular for Cloud specific step logic.
Also an option to consider once this gets mature would be rather than encapsulate into hidden step struct functions, could use nested user facing commands on top, so if required the user could tectonic-cli step igntion, this should be pretty straight forward if we do the right abstractions

sym3tri · 2018-02-02T12:14:45Z

installer/pkg/tectonic/buildstate.go

+	var err error
+	pwd, err := os.Getwd()
+	if err != nil {
+		log.Fatalf("Failed to current directory because: %v", err)


missing a word in this sentence "failed to ____ current directory..."

squat

A few questions/concerns

squat · 2018-02-02T12:20:50Z

installer/pkg/workflow/workflow.go

+
+type Step func(*metadata) error
+
+// Context represents the state of a workflow


What “Context”?

leftover. needs fixing.

squat · 2018-02-02T12:21:34Z

installer/pkg/workflow/workflow.go

+type Step func(*metadata) error
+
+// Context represents the state of a workflow
+type SimpleWorkflow struct {


This type should be unexported. The exports type is the interface. No one outside of the workflow package should care about or depend on this type at all

squat · 2018-02-02T12:29:18Z

installer/pkg/workflow/install.go

+}
+
+func terraformPrepareStep(m *metadata) error {
+	if m.statePath == "" {


It seems odd to me that the steps mutate the metadata. This should happen in the install workflow constructor IMO

In my mind that was the whole point of passing the metadata object around.
It was supposed to provide a vehicle for passing data between steps, without coupling the steps to each other.

If you want to decouple the steps then the metadata must be an invariant that is completely filled during construction. This implementation is the opposite. IMO. All steps that require the statePath metadata are now dependent on this step.

enxebre · 2018-02-05T09:44:10Z

installer/pkg/workflow/install.go

+
+func terraformApplyStep(m *metadata) error {
+	log.Printf("Installation is running...")
+	tfInit := exec.Command("terraform", "apply", tectonic.FindTemplatesForType("aws")) // TODO: get from cluster config


tfInit sounds like a carry over name from previews terraformInitStep
in a follow up PR we can probably

func runTfCommand(buildPath string, args ...string) error { tfCommand := exec.Command("terraform", args...) tfCommand.Dir = buildPath tfCommand.Stdin = os.Stdin tfCommand.Stdout = os.Stdout tfCommand.Stderr = os.Stderr err := tfCommand.Run() if err != nil { return err } return nil }

I deliberately chose to stay away from abstracting this just yet, because I wanted to get a feel of what the interface to the TF runner abstraction would need to look like once we have a few workflows in place.

I'll incorporate your suggestion and we'll watch how it maps to some real workflows and evolve it from there if needed.

squat · 2018-02-05T13:01:41Z

Go Lint is failing due to improper commenting on a few funcs:
NewDestroyWorkflow
NewInstallWorkflow
ClusterNameFromConfig
FindTemplatesForType

othewise looks good for now

squat

lgtm. Want to squash? otherwise merge on green. we can do followups to clean up the bazel files in another pr

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

The typo is from 1b4bb62 (Cli tool, 2018-02-05, coreos/tectonic-installer#2806).

This code is descended from terraformPrepareStep, which landed in 1b4bb62 (Cli tool, 2018-02-05, coreos/tectonic-installer#2806). The workspace version was factored out into copyFile in 2b82fbe (installer: Integrate multistep cli with configuration, 2018-02-16, coreos/tectonic-installer#2960). The config-generator version was copy/pasted (or independently developed?) in 1d54899 (installer/pkg/config-generator/tls: generate root CA's with go, 2018-06-28, coreos/tectonic-installer#3316). This commit DRYs that up by pulling the duplicate code out into a shared package. I've also changed O_RDWR to O_WRONLY. The O_RDWR is originally from 1b4bb62, but we do not require the ability to read from the target file. Also add a unit test to exercise this code.

alexsomesan requested a review from spangenberg January 23, 2018 15:11

squat suggested changes Jan 23, 2018

View reviewed changes

squat reviewed Jan 23, 2018

View reviewed changes

enxebre mentioned this pull request Jan 29, 2018

Multistep bootstrap - WIP #2802

Closed

alexsomesan force-pushed the cli-tool branch from 5bec08e to b9b4d7a Compare January 29, 2018 16:39

cpanato reviewed Jan 30, 2018

View reviewed changes

alexsomesan force-pushed the cli-tool branch from b13d46c to de67f6c Compare January 30, 2018 19:44

alexsomesan force-pushed the ut2-integration branch from a060f4e to b6b3195 Compare January 30, 2018 19:49

sym3tri reviewed Feb 2, 2018

View reviewed changes

squat reviewed Feb 2, 2018

View reviewed changes

enxebre reviewed Feb 5, 2018

View reviewed changes

squat approved these changes Feb 5, 2018

View reviewed changes

alexsomesan added 3 commits February 5, 2018 15:47

CLI MVP

5a52750

Kingpin bazel build

2d81ed2

Bazel: per-arch targets and top-level alias

c6ab1c0

alexsomesan force-pushed the cli-tool branch from bd4dde8 to c6ab1c0 Compare February 5, 2018 14:49

alexsomesan merged commit 334bd7c into coreos:ut2-integration Feb 5, 2018

alexsomesan deleted the cli-tool branch February 5, 2018 15:06

alexsomesan added a commit to alexsomesan/tectonic-installer that referenced this pull request Feb 5, 2018

Cli tool (coreos#2806)

38ddc5d

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

alexsomesan added a commit to alexsomesan/tectonic-installer that referenced this pull request Feb 5, 2018

Cli tool (coreos#2806)

df0a3d6

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

enxebre pushed a commit that referenced this pull request Feb 6, 2018

Cli tool (#2806)

417b10b

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

enxebre pushed a commit to enxebre/tectonic-installer that referenced this pull request Feb 9, 2018

Cli tool (coreos#2806)

c750c48

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

enxebre pushed a commit to enxebre/tectonic-installer that referenced this pull request Feb 15, 2018

Cli tool (coreos#2806)

b939072

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

alexsomesan added a commit to alexsomesan/tectonic-installer that referenced this pull request Feb 21, 2018

Cli tool (coreos#2806)

bf3acee

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

mxinden pushed a commit that referenced this pull request Feb 27, 2018

Cli tool (#2806)

1b4bb62

* CLI MVP * Kingpin bazel build * Bazel: per-arch targets and top-level alias

wking added a commit to wking/openshift-installer that referenced this pull request Jul 9, 2018

installer/pkg/workflow/workflow: Fix "taked" -> "take" comment typo

cb42fcd

The typo is from 1b4bb62 (Cli tool, 2018-02-05, coreos/tectonic-installer#2806).

wking mentioned this pull request Jul 9, 2018

installer/pkg/workflow/workflow: Fix "taked" -> "take" comment typo openshift/installer#36

Merged

wking mentioned this pull request Jul 9, 2018

installer/pkg/copy: Factor out Copy into its own package openshift/installer#38

Merged


		type Step func(*metadata) error

		// Context represents the state of a workflow

Cli tool #2806

Cli tool #2806

Uh oh!

Conversation

alexsomesan commented Jan 23, 2018

Uh oh!

coreosbot commented Jan 23, 2018

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexsomesan commented Jan 23, 2018

Uh oh!

squat commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpanato Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cpanato Jan 30, 2018 •

edited

Loading

enxebre commented Feb 1, 2018 •

edited

Loading

enxebre Feb 5, 2018 •

edited

Loading