Skip to content

*: Pass the master, worker and bootstrap ignition content to the terraform. #301

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

Jump to bottom
Closed
yifan-gu wants to merge 2 commits into from
Closed

*: Pass the master, worker and bootstrap ignition content to the terraform. #301

yifan-gu wants to merge 2 commits into from

Conversation

@yifan-gu
Copy link
Contributor

@yifan-gu yifan-gu commented Sep 21, 2018

Previously, the installer binary generates the ignitions for master
and worker nodes, and pass the filename to the terraform.

However, in the new installer binary, we will instead pass the content
of the ignition files to terraform.

Also adds an option to use the bootstrap ignition from the tfvars file directly.

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 21, 2018
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 21, 2018
abhinavdahiya
abhinavdahiya reviewed Sep 21, 2018
View reviewed changes
installer/pkg/config-generator/ignition.go Outdated
Copy link
Contributor

@abhinavdahiya abhinavdahiya Sep 21, 2018

Choose a reason for hiding this comment

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

we shouldn't be indenting as it will be part of the terraform.tfvars

abhinavdahiya
abhinavdahiya reviewed Sep 21, 2018
View reviewed changes
pkg/types/config/cluster.go Outdated
Copy link
Contributor

@abhinavdahiya abhinavdahiya Sep 21, 2018

Choose a reason for hiding this comment

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

changed yaml:"-" to yaml:"<something>"; We might wanna keep it ignored.

@yifan-gu yifan-gu force-pushed the ignition_by_value branch 7 times, most recently from d499821 to f759287 Compare September 21, 2018 20:12
@crawford
Copy link
Contributor

crawford commented Sep 21, 2018

Can you fix up the commit message on the one? *.*: is a strange scope. *: would be more appropriate.

@yifan-gu yifan-gu force-pushed the ignition_by_value branch 2 times, most recently from d9c525d to adb9063 Compare September 21, 2018 21:18
@yifan-gu yifan-gu changed the title *.*: Pass the master, worker and bootstrap ignition content to the terraform. *: Pass the master, worker and bootstrap ignition content to the terraform. Sep 21, 2018
*: Pass the master and worker ignition content to the terraform.
00f105e 
Previously, the installer binary generates the ignitions for master
and worker nodes, and pass the filename to the terraform.

However, in the new installer binary, we will instead pass the content
of the ignition files to terraform.
@crawford
Copy link
Contributor

crawford commented Sep 21, 2018

This requires openshift/release#1630.

steps/infra: Use the bootstrap ignition in the tfvars if its set.
2e340f1 
If the "ignition_bootstrap" is non-empty in the terraform.tfvars,
then use it as the content for the bootstrap ignition.

This will enable the new installer binary to launch a cluster.
@crawford crawford mentioned this pull request Sep 23, 2018
Merged
@crawford
Copy link
Contributor

crawford commented Sep 23, 2018

/retest

@crawford
Copy link
Contributor

crawford commented Sep 23, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2018
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, yifan-gu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 23, 2018

@yifan-gu: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws 2e340f1 link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@wking wking added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2018
@crawford
Copy link
Contributor

crawford commented Sep 23, 2018

(Accidentally) superseded by #289.

/close

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 23, 2018

@crawford: Closing this PR.

Details

In response to this:

(Accidentally) superseded by #289.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@yifan-gu yifan-gu deleted the ignition_by_value branch September 24, 2018 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crawford crawford Awaiting requested review from crawford

@rajatchopra rajatchopra Awaiting requested review from rajatchopra

1 more reviewer

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@crawford crawford

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yifan-gu @crawford @openshift-ci-robot @abhinavdahiya @wking