Refactor workflow by enxebre · Pull Request #3197 · coreos/tectonic-installer

enxebre · 2018-04-20T18:24:53Z

The workflow package with the metadata shared by reference across steps has given us high flexibility and ability for making changes and prototype very quickly. As product requirements get clearer and we introduce a second platform we should define clearer boundaries, stronger interfaces and refactor the package.
This proposes a refactor into a more object oriented and hermetic flavour, less prone to side effects. The main logic is in cluster.go

coreosbot · 2018-04-20T18:24:54Z

Can one of the admins verify this patch?

squeed · 2018-04-23T09:54:50Z

installer/pkg/config-generator/generator.go


+// GenerateKubeConfig returns, if successful, the cluster kubernetes config
+func (c ConfigGenerator) GenerateKubeConfig(workspace string) error {
+	//configGenerator := configgenerator.New(c.config)


delete these comments?

squeed · 2018-04-23T09:57:37Z

installer/pkg/workflow/cluster.go

+func (c Cluster) runDestroyStep(step string, extraArgs ...string) error {
+	if !hasStateFile(c.workspace, step) {
+		// there is no statefile, therefore nothing to destroy for this step
+		return nil


does it make sense to log a warning here? or is this an expected state?

squeed · 2018-04-23T09:57:43Z

installer/pkg/workflow/cluster.go

+	}
+	return tfApply(c.workspace, step, templateDir, extraArgs...)
+
+	return nil


extra return

squeed · 2018-04-23T10:04:47Z

installer/pkg/workflow/cluster.go

+	if err := c.runInstallStep(assetsStep); err != nil {
+		return err
+	}
+	if err := c.generateKubeConfig(); err != nil {


just a FYI, I had to move this before the runInstallStep(assetsStep) as part of #3183.

alexsomesan · 2018-04-23T11:34:30Z

Wearing the Ed hat, I'd have to ask which sprint / epic issue does this change tie into?

alexsomesan

The workspace concept this change introduces is quite foreign to the problem domain that the installer tries to solve - that is, installing Kubernetes (Tectonic / OpenShift) clusters.

Let's try to take a deeper look at it and see how it fits.

Let me pose the following question:

As a user of this tool, I want to install a Tectonic / OpenShift cluster. Previously I could install a cluster without having to supply a workspace as input.
How does supplying a workspace name as an input improve my experience in achieving the goal of installing a cluster?

On the other hand, as a user of this tool, how does not supplying a workspace as an input hinder me from achieving the goal of installing the cluster?

Let's try to answer these questions from the perspective of a user expecting a running cluster after using this tool.

squeed · 2018-04-23T11:47:16Z

installer/pkg/workflow/terraform.go

+// returns the directory containing templates for a given step. If platform is
+// specified, it looks for a subdirectory with platform first, falling back if
+// there are no platform-specific templates for that step
+func findStepTemplates(stepName, platform string) (string, error) {


One little thing: could you convert platform to lower case? Otherwise I'll have to update that in a separate PR.

https://github.com/coreos/tectonic-installer/pull/3197/files#diff-e22bb80d035913a0a75945c1e983a35bR88

alexsomesan · 2018-04-23T11:48:58Z

As a side note, I'd prefer that major changes like this be discussed briefly as proposals before actually putting any code down.
This is to minimize friction in incorporating the change and making sure it's in the right shape to get in.

squeed · 2018-04-23T11:49:54Z

installer/pkg/workflow/cluster.go

+}
+
+// Install runs, if successful, the steps to install a cluster
+func (c Cluster) Install() error {


This is a lot cleaner, nice.

enxebre · 2018-04-23T11:59:03Z

hey @alexsomesan on the workspace questions, there's actually no new concept, it's just renaming clusterDir to workspace. In addition (but it's no necessary), I also added it as a flag so the user can use the same source config file to init different clusters

squeed · 2018-04-23T12:02:16Z

As part of this, it might be nice to abstract Cluster behind an interface; that would help with multi-platform.

enxebre · 2018-04-23T12:35:36Z

@squeed I thought about the interface but wanted to keep as simple as possible for now. Let's add the abstraction if needed with the introduction of the second platform. It also crossed my mind to have a infraImpl interface composing the cluster struct to implement the desired functionality. Once we have two will be easier to abstract more if required

squat · 2018-05-09T11:26:37Z

installer/cmd/tectonic/main.go

+	var c *workflow.Cluster
+	var err error
+
+	newCluster := func(clusterInstallDirFlag string) *workflow.Cluster {


can we pull this function definition out of the main func? there is no need to do this inline.

squat · 2018-05-09T11:27:45Z

installer/cmd/tectonic/main.go

+	var c *workflow.Cluster
+	var err error
+
+	newCluster := func(clusterInstallDirFlag string) *workflow.Cluster {


clusterInstallDirFlag is a funny name for this parameter. the function should not care if it is a flag or an argument or how the string was provided to the function. Could simply be named dir.

squat · 2018-05-09T11:29:54Z

installer/cmd/tectonic/main.go

+	var err error
+
+	newCluster := func(clusterInstallDirFlag string) *workflow.Cluster {
+		l, err := log.ParseLevel(*logLevel)


creating a cluster should be separate from parsing the log level. In fact, this change introduces a new behavior where log levels are not parsed for the convert command since that command does not execute this newCluster func. I think parsing the log level should be done outside of this func.

squat · 2018-05-09T11:32:30Z

installer/pkg/config-generator/generator.go

+func (c ConfigGenerator) GenerateClusterConfigMaps(workspace string) error {
+	clusterGeneratedPath := filepath.Join(workspace, generatedPath)
+	if err := os.MkdirAll(clusterGeneratedPath, os.ModeDir|0755); err != nil {
+		return fmt.Errorf("Failed to create cluster generated directory at %s", clusterGeneratedPath)


We typically make errors lower-case so they can be wrapped.

squat · 2018-05-09T11:33:09Z

installer/pkg/config-generator/generator.go

+
+	kubePath := filepath.Join(workspace, kubeSystemPath)
+	if err := os.MkdirAll(kubePath, os.ModeDir|0755); err != nil {
+		return fmt.Errorf("Failed to create manifests directory at %s", kubePath)


same as above

squat · 2018-05-09T11:33:18Z

installer/pkg/config-generator/generator.go

+
+	tectonicPath := filepath.Join(workspace, tectonicSystemPath)
+	if err := os.MkdirAll(tectonicPath, os.ModeDir|0755); err != nil {
+		return fmt.Errorf("Failed to create tectonic directory at %s", tectonicPath)


same as above

squat · 2018-05-09T11:35:38Z

installer/pkg/workflow/cluster.go

+// InitWorkspace generates, if successful, a workspace folder with the config.yaml
+func InitWorkspace(sourceConfigFilePath, workspaceName string) error {
+	if sourceConfigFilePath == "" {
+		errors.New("no cluster sourceConfigFilePath given for instantiating new cluster")


should this be a return? currently this line evaluates the errors.New func but does nothing with the returned value

squat · 2018-05-09T11:35:46Z

installer/pkg/workflow/cluster.go

+		errors.New("no cluster sourceConfigFilePath given for instantiating new cluster")
+	}
+	if workspaceName == "" {
+		errors.New("no cluster sourceConfigFilePath given for instantiating new cluster")


same as above

squat · 2018-05-09T11:36:13Z

installer/pkg/workflow/cluster.go

+// It ensures cluster.config and the tfvars file are always up to date with config.yaml
+func NewCluster(workspace string) (*Cluster, error) {
+	if workspace == "" {
+		errors.New("no workspace dir given for new cluster")


same as above

enxebre · 2018-05-09T11:38:06Z

@squat thanks for having a look. Let's close this for now as more internal discussions is needed about the approach before going deeper into the code. cc @alexsomesan

squat · 2018-05-09T11:39:47Z

installer/pkg/workflow/cluster.go

+// want that to happen atomically with generateTerraformVariables
+func readClusterConfig(workspace string) (*config.Cluster, error) {
+	if workspace == "" {
+		errors.New("no workspace dir given for reading config")


This statement does not do anything with the returned value from errors.New. Did you mean to return here?

squat · 2018-05-09T11:40:32Z

installer/pkg/workflow/cluster.go

+
+func (c Cluster) runDestroyStep(step string, extraArgs ...string) error {
+	if !hasStateFile(c.workspace, step) {
+		log.Warningf("there is no statefile, therefore nothing to destroy for the step %s within %s", step, c.workspace)


Log lines typically begin with a capital letter

squat · 2018-05-09T11:46:03Z

installer/pkg/workflow/cluster.go

+	yamlContent, err := yaml.Marshal(internalCfg)
+	internalFileContent := []byte("# Do not touch, auto-generated\n")
+	internalFileContent = append(internalFileContent, yamlContent...)
+	if err != nil {


it's typical to put the error check immediately after the error declaration

enxebre force-pushed the refactor branch from fe677cd to 9fa3874 Compare April 23, 2018 07:48

enxebre requested review from alexsomesan, spangenberg, squat and squeed April 23, 2018 07:49

enxebre changed the title ~~Refactor~~ Refactor workflow Apr 23, 2018

enxebre force-pushed the refactor branch from 9fa3874 to d087185 Compare April 23, 2018 08:29

squeed reviewed Apr 23, 2018

View reviewed changes

installer/pkg/workflow/cluster.go Outdated

}

return tfApply(c.workspace, step, templateDir, extraArgs...)

return nil

Copy link

squeed Apr 23, 2018

Choose a reason for hiding this comment

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

extra return

squeed reviewed Apr 23, 2018

View reviewed changes

alexsomesan reviewed Apr 23, 2018

View reviewed changes

squeed reviewed Apr 23, 2018

View reviewed changes

enxebre force-pushed the refactor branch from d087185 to 5508eb3 Compare April 23, 2018 12:26

enxebre force-pushed the refactor branch 2 times, most recently from b425ddb to e95f68c Compare April 23, 2018 12:50

refactor workflow package

2136650

enxebre force-pushed the refactor branch from e95f68c to 2136650 Compare April 23, 2018 12:58

squat reviewed May 9, 2018

View reviewed changes

enxebre closed this May 9, 2018

squat reviewed May 9, 2018

View reviewed changes

Conversation

enxebre commented Apr 20, 2018

Uh oh!

coreosbot commented Apr 20, 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!

alexsomesan commented Apr 23, 2018

Uh oh!

alexsomesan left a comment • 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!

alexsomesan commented Apr 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre commented Apr 23, 2018

Uh oh!

squeed commented Apr 23, 2018

Uh oh!

enxebre commented Apr 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squat May 9, 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!

Choose a reason for hiding this comment

Uh oh!

enxebre commented May 9, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexsomesan left a comment •

edited

Loading

squat May 9, 2018 •

edited

Loading