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

modules: unify ignition across platforms#1743

Merged
s-urbaniak merged 13 commits intocoreos:masterfrom
s-urbaniak:unify-ign
Aug 29, 2017
Merged

modules: unify ignition across platforms#1743
s-urbaniak merged 13 commits intocoreos:masterfrom
s-urbaniak:unify-ign

Conversation

@s-urbaniak
Copy link
Contributor

Fixes partially INST-132

@mxinden
Copy link
Contributor

mxinden commented Aug 22, 2017

ok to test

@mxinden
Copy link
Contributor

mxinden commented Aug 22, 2017

ok to test

@s-urbaniak s-urbaniak force-pushed the unify-ign branch 2 times, most recently from 98d62fd to d785d36 Compare August 22, 2017 13:50

data "ignition_file" "max_user_watches" {
filesystem = "root"
path = "/etc/sysctl.d/max-user-watches.conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion while at it: use a nn-foo.conf name scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucab ack, good idea!

@mxinden
Copy link
Contributor

mxinden commented Aug 22, 2017

@s-urbaniak Tests run properly again.

@s-urbaniak
Copy link
Contributor Author

@mxinden thanks a lot!

@s-urbaniak
Copy link
Contributor Author

ok to test

2 similar comments
@s-urbaniak
Copy link
Contributor Author

ok to test

@s-urbaniak
Copy link
Contributor Author

ok to test

@s-urbaniak s-urbaniak force-pushed the unify-ign branch 3 times, most recently from 6decf17 to 441209f Compare August 24, 2017 11:29
kubeconfig_s3_location = "${aws_s3_bucket_object.kubeconfig.bucket}/${aws_s3_bucket_object.kubeconfig.key}"

kubelet_exec_start_pre = <<EOF
ExecStartPre=/usr/bin/bash -c "/opt/s3-puller.sh ${aws_s3_bucket_object.kubeconfig.bucket}/${aws_s3_bucket_object.kubeconfig.key} /etc/kubernetes/kubeconfig"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit (maybe for later): I think this bash -c is useless here and just a leftover from a similar line in another service which is doing a redirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is indeed a valid point, I factored bash -c out in this PR.

@s-urbaniak
Copy link
Contributor Author

ok to test

@s-urbaniak s-urbaniak force-pushed the unify-ign branch 5 times, most recently from d4ddfd9 to 2c5aca3 Compare August 25, 2017 14:31
Fixes partially INST-132
Sergiusz Urbaniak added 3 commits August 28, 2017 09:50
@s-urbaniak s-urbaniak changed the title [WIP] modules: unify ignition across platforms modules: unify ignition across platforms Aug 28, 2017
Sergiusz Urbaniak added 2 commits August 28, 2017 10:19
This adds the same docker options dropin as all the other platforms.

Fixes INST-38
@s-urbaniak
Copy link
Contributor Author

PTAL @alexsomesan

private_key = "${var.tectonic_vmware_ssh_private_key_path}"
image_re = "${var.tectonic_image_re}"

ign_docker_dropin_id = "${module.ignition_masters.docker_dropin_id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ignition_masters/ignition_workers/ (here and lines below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed

@s-urbaniak
Copy link
Contributor Author

ok to test

alexsomesan
alexsomesan previously approved these changes Aug 28, 2017
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!
Glad to see this taking shape.

Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

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

(Casual bystander) LGTM. Checked on my bare-metal playground.

@s-urbaniak
Copy link
Contributor Author

@lucab your bystanding is highly appreciated :-D

@s-urbaniak
Copy link
Contributor Author

ok to test

1 similar comment
@s-urbaniak
Copy link
Contributor Author

ok to test

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.

Still look good 👍

@s-urbaniak
Copy link
Contributor Author

ok to test

@cpanato cpanato closed this Aug 28, 2017
@cpanato cpanato reopened this Aug 28, 2017
@cpanato
Copy link
Contributor

cpanato commented Aug 29, 2017

all green now @s-urbaniak

@s-urbaniak
Copy link
Contributor Author

@cpanato thanks and kudos for helping with improving CI!

@s-urbaniak s-urbaniak merged commit c18506a into coreos:master Aug 29, 2017
squat pushed a commit to squat/tectonic-installer that referenced this pull request Sep 25, 2017
* modules/ignition: unify max-user-watches

Fixes partially INST-132

* modules/ignition: unify docker dropin

Fixes partially INST-132

* modules/ignition: unify kubelet

* modules/ignition: unify locksmithd service

* modules/ignition: unify kubelet.env

* modules/ignition: unify tx-off service

* modules/ignition: unify 66-azure-storage-rules

* moduls/azure: disable stale tectonic service on workers

... it is not needed there, only masters are being used for bootstrapping.

* modules/azure: remove stale azure_udev_rules definition

* platforms/metal: unify dockeropts with other platforms

This adds the same docker options dropin as all the other platforms.

Fixes INST-38

* platforms/*: s/ignition_masters/ignition_workers

* modules/ignition: apply nn-foo.conf scheme for max-user-watches.conf

Fixes coreos#1743 (comment)

* modules/vmware: remove stale kubelet-env declaration
nreisbeck added a commit to nreisbeck/tectonic-installer that referenced this pull request Oct 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants