-
Notifications
You must be signed in to change notification settings - Fork 266
*/rm-assets: activate using path activation #2421
Conversation
Can one of the admins verify this patch? |
da2e446
to
80b1c2f
Compare
80b1c2f
to
4844882
Compare
/cc @squat |
@@ -1,7 +1,7 @@ | |||
[Unit] | |||
Description=Clean up install assets from S3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't we just run rm-assets.sh
as a ExecStartPost in bootkube tectonic service to alleviate dependency hassle? then each platform implements the cleaning up as they wish (basically adding or not the cloud storage api call)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some platforms don't have the notion of rm-assets.sh
which semantically only deletes remote assets.
Local assets are being removed inside tectonic.sh
but here I believe another service makes sense.
Platforms not supporting pull semantics (openstack, vmware, baremetal) don't need this service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also internally discussed the idea of running a one-shot k8s Job post-install, but this is better punted to a later refactoring (track 2), as this adds yet more manifest skew.
data "ignition_systemd_unit" "rm_assets" { | ||
name = "rm-assets.service" | ||
enable = "${var.assets_location != "" ? true : false}" | ||
enable = false | ||
content = "${file("${path.module}/resources/services/rm-assets.service")}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this meant to be always disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes the idea is for it to be path-activated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
retest this please |
@squat PTAL |
@@ -15,6 +15,10 @@ resource "aws_s3_bucket" "tectonic" { | |||
"KubernetesCluster", "${var.tectonic_cluster_name}", | |||
"tectonicClusterID", "${module.tectonic.cluster_id}" | |||
), var.tectonic_aws_extra_tags)}" | |||
|
|||
lifecycle { | |||
ignore_changes = ["*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to ignore lifecycle changes on the bucket too even though only the one file is changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I did experimentation locally and removed one file from the bucket, this affected also the bucket itself, hence I had to add this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small question, but LGTM
No description provided.