Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

installer: Integrate multistep cli with configuration #2960

Merged
spangenberg merged 1 commit into from
Feb 16, 2018
Merged

installer: Integrate multistep cli with configuration #2960

spangenberg merged 1 commit into from
Feb 16, 2018

Conversation

@spangenberg
Copy link
Contributor

@spangenberg spangenberg commented Feb 15, 2018

No description provided.

@spangenberg spangenberg self-assigned this Feb 15, 2018
@coreosbot
Copy link

coreosbot commented Feb 15, 2018

Can one of the admins verify this patch?

@spangenberg spangenberg force-pushed the ut2-integration branch 2 times, most recently from e87e8b9 to 29bc125 Compare February 15, 2018 23:41
enxebre
enxebre reviewed Feb 16, 2018
View reviewed changes
installer/pkg/workflow/terraform.go Outdated
return err
}
return nil
const (
Copy link
Contributor

@enxebre enxebre Feb 16, 2018

Choose a reason for hiding this comment

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

I'd rather move this constants and findTemplatesForStep to a different package may be utils or step, and keep terraform workflow agnostic, with which is purely terraform

Copy link
Contributor Author

@spangenberg spangenberg Feb 16, 2018

Choose a reason for hiding this comment

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

Makes sense, will change that.

@enxebre
Copy link
Contributor

enxebre commented Feb 16, 2018 •
edited
Loading

just had a quick look, is looking great to me.

  • Can you share an example config to test this?
  • As there's assets that rely on terraform yet, e.g ignition, bootkube.sh, tectonic.sh, ncg ncg-ignition-config, for now we'll need to consolidate the output path of the kube yamls generated here with the generated by terraform, and make sure it keep satisfying the bootstrap step assumptions to upload and deploy all of them

enxebre
enxebre reviewed Feb 16, 2018
View reviewed changes
installer/pkg/workflow/install.go
steps: []Step{
terraformPrepareStep,
assetsStep,
readClusterConfigStep,
Copy link
Contributor

@enxebre enxebre Feb 16, 2018

Choose a reason for hiding this comment

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

why is the readClusterConfigStep step needed?

Copy link
Contributor Author

@spangenberg spangenberg Feb 16, 2018

Choose a reason for hiding this comment

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

Metadata always just gets the cluster dir:

metadata: metadata{clusterDir: clusterDir}

The readClusterConfigStep takes care of ensuring that the config gets parsed, e.g. for cluster Name or Platform.

@spangenberg
Copy link
Contributor Author

spangenberg commented Feb 16, 2018

@enxebre can you point me towards the output path where you want the operator yml's to be. I'll include a config.yml in the contrib for the moment.

@enxebre
Copy link
Contributor

enxebre commented Feb 16, 2018 •
edited
Loading

@spangenberg the casuistic is as follows:

@spangenberg spangenberg force-pushed the ut2-integration branch 4 times, most recently from 132fe2a to 9f2142a Compare February 16, 2018 15:02
alexsomesan
alexsomesan approved these changes Feb 16, 2018
View reviewed changes
Copy link
Contributor

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

Looks good overall.

@enxebre
Copy link
Contributor

enxebre commented Feb 16, 2018 •
edited
Loading

@spangenberg thanks a lot, this is pretty awesome work!
I'm currently deploying from this branch doing smoke functional testing
Some gotchas:

  • EtcdCount is being ignored, after a quick look, probably because is expecting a string https://github.com/coreos/tectonic-installer/blob/ut2-integration/installer/pkg/terraform-generator/tectonic.go#L27?
  • Kube yamls are generated with multiple blanks
  • The way this is at the moment, the kube yaml files will be overridden by the "assets" step, as terraform will override the whole "generated" folder. As a transition while we have to share the assets generation responsibility we should probably
    run init
    run assets
    mv init yaml into expected folder
    Would you mind to add TODO note to spot that out?
  • In follow ups we need to add high coverage for unit test the templates generation from different config files, which will give a huge benefit at the time of making changes

@spangenberg
Copy link
Contributor Author

spangenberg commented Feb 16, 2018

@enxebre thanks, this sounds fantastic.

I changed the workflow order that the config yamls will be generated after assets step was run.
This should circumvent the issues you encountered.
I will try to target the config issues in another PR.

@spangenberg spangenberg merged commit 7f7e7b8 into coreos:ut2-integration Feb 16, 2018
alexsomesan pushed a commit to alexsomesan/tectonic-installer that referenced this pull request Feb 21, 2018
@spangenberg @alexsomesan
installer: Integrate multistep cli with configuration (coreos#2960)
e55db33
mxinden pushed a commit that referenced this pull request Feb 27, 2018
@spangenberg @mxinden
installer: Integrate multistep cli with configuration (#2960)
2b82fbe
@wking wking mentioned this pull request Jul 9, 2018
Merged
@wking wking mentioned this pull request Jul 9, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this pull request Jul 9, 2018
@wking
installer/pkg: Replace writeFile with ioutil.WriteFile
9912b19 
The writeFile helpers are from 2b82fbe (installer: Integrate
multistep cli with configuration, 2018-02-16,
coreos/tectonic-installer#2960) and 461ff5f (cli: add support for
user ignition files, 2018-03-23, coreos/tectonic-installer#3120).  But
ioutil's function has almost the same signature.  Remove our local
versions and use the stdlib's instead.

The \n addition is because ioutil.WriteFile does not append newlines
(which workflow.writeFile did via Fprintln).

I've used os.Create's 0666 [1] in most cases; callers can set a umask
if they want to restrict that.  The exception is in
generatePrivateKey, where I've set 0600 to avoid leaking a private key
even in the presence of a weak umask.

[1]: https://golang.org/pkg/os/#Create
wking added a commit to wking/openshift-installer that referenced this pull request Jul 16, 2018
@wking
installer/pkg/copy: Factor out Copy into its own package
9abf7a3 
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.
wking added a commit to wking/openshift-installer that referenced this pull request Jul 20, 2018
@wking
installer/cmd/tectonic/main: List 'full' first among 'install' subcom…
1586cc6 
…mands

2b82fbe (installer: Integrate multistep cli with configuration,
2018-02-16, coreos/tectonic-installer#2960) shifted it into the middle
of the smaller install subcommands.  But because:

* those smaller subcommands are, with the exception of newtls, subsets
  of full, and
* full is the default action,

I think it makes more sense to move it back to the top.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@squat squat Awaiting requested review from squat

2 more reviewers

@enxebre enxebre enxebre left review comments

@alexsomesan alexsomesan alexsomesan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@spangenberg spangenberg

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@spangenberg @coreosbot @enxebre @alexsomesan