Skip to content

Documentation/design: add launch design #51

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
Merged
openshift-merge-robot merged 1 commit into from
Jul 17, 2018
Merged

Documentation/design: add launch design #51

openshift-merge-robot merged 1 commit into from
Jul 17, 2018

Conversation

@abhinavdahiya
Copy link
Contributor

@abhinavdahiya abhinavdahiya commented Jul 12, 2018

Add design doc for launch step.

Also includes detail on bootstrap cluster flow decided in Option B

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 12, 2018
@abhinavdahiya abhinavdahiya force-pushed the design_launch branch 2 times, most recently from 78cc555 to 9d58683 Compare July 12, 2018 16:37
yifan-gu
yifan-gu reviewed Jul 12, 2018
View reviewed changes
Documentation/design/launchphase.md Outdated
Copy link
Contributor

@yifan-gu yifan-gu Jul 12, 2018

Choose a reason for hiding this comment

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

Any reason why? As today the tectonic installer is idempotent right?

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Jul 12, 2018

Choose a reason for hiding this comment

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

today

installer/installer/pkg/workflow/install.go

Lines 129 to 134 in ae41b0a

func installTNCCNAMEStep(m *metadata) error {
if !clusterIsBootstrapped(m.clusterDir) {
return createTNCCNAME(m)
}
return nil
}
condition skips CNAME dance.

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Jul 12, 2018

Choose a reason for hiding this comment

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

It is not a requirement as we discussed previously. If the launch errors on re-run, it is acceptable.

Copy link
Contributor

@crawford crawford Jul 12, 2018

Choose a reason for hiding this comment

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

As today the tectonic installer is idempotent right?

It is most definitely not idempotent. Running the installer a second time is one of the best ways to screw up your cluster.

Copy link
Contributor

@yifan-gu yifan-gu Jul 13, 2018

Choose a reason for hiding this comment

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

I did it couple times when I came back to my terminal from interruption, didn't remember seeing errors. Anyway whatever is easier is fine to me.

wking
wking reviewed Jul 14, 2018
View reviewed changes
Documentation/design/launchphase.md Outdated
Copy link
Member

@wking wking Jul 14, 2018

Choose a reason for hiding this comment

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

"Th" -> "The". And you can probably drop the /.

yifan-gu
yifan-gu approved these changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@yifan-gu yifan-gu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2018
@openshift-merge-robot openshift-merge-robot merged commit 8f39493 into openshift:master Jul 17, 2018
wking added a commit to wking/openshift-installer that referenced this pull request Jul 23, 2018
@wking
installer/pkg: Drop "ignition-" from "ignition-*.ign" filenames
6290477 
The ".ign" suffix is sufficient to identify these as ignition
filenames.  The old filenames were from 461ff5f (cli: add support for
user ignition files, 2018-03-23, coreos/tectonic-installer#3120),
which does not motivate the "ingition-" prefix.

This gets us closer to the filenames discussed in bf830cc
(Documentation/design: add prepare design, 2018-07-11, openshift#50) and
b111829 (Documentation/design: add launch design, 2018-07-11, openshift#51).
@wking wking mentioned this pull request Jul 23, 2018
Merged
stbenjam pushed a commit to stbenjam/installer that referenced this pull request Feb 10, 2021
@openshift-merge-robot
Merge pull request openshift#51 from guillaumerose/master
4c9b580 
add current profile annotations to CVO manifests
clnperez pushed a commit to clnperez/installer that referenced this pull request Sep 2, 2021
@23TNC @cklokman @clnperez
Adding support for a vector of api key env variables (openshift#51)
0792b55 
This should add support for a list of env vars for the API key, and can be used for other variables in the future.

Signed-off-by: wolf <[email protected]>

Co-authored-by: wolf <[email protected]>
clnperez pushed a commit to clnperez/installer that referenced this pull request Sep 7, 2021
@23TNC @cklokman
Adding support for a vector of api key env variables (openshift#51)
0557c15 
This should add support for a list of env vars for the API key, and can be used for other variables in the future.

Signed-off-by: wolf <[email protected]>

Co-authored-by: wolf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wking wking wking left review comments

@crawford crawford Awaiting requested review from crawford

@aaronlevy aaronlevy Awaiting requested review from aaronlevy

+1 more reviewer

@yifan-gu yifan-gu yifan-gu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@yifan-gu yifan-gu

Labels

lgtm Indicates that a PR is ready to be merged. retest-not-required-docs-only size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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