-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
feat: Map hostmount volumes through to config.toml #12
feat: Map hostmount volumes through to config.toml #12
Conversation
@MeNsaaH can I ask approval to begin the workflows to pre-qualify this PR? This is the first of three PRs I'd like to submit to extend the coverage of TF over the runner config if you're willing to accept them. Thanks! |
config.tf
Outdated
%{~for name, config in var.build_job_hostmounts~} | ||
[[runners.kubernetes.volumes.host_path]] | ||
name = "${name}" | ||
mount_path = "${config.path}" |
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.
@chickenandpork I'm thinking of cases where someone wants to mount a host path to a different path in the container. Would it be better to make this config.container_path
and config.host_path
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.
yeah, that makes sense and is quick to add
@@ -105,6 +105,11 @@ variable "local_cache_dir" { | |||
description = "Path on nodes for caching" | |||
} | |||
|
|||
variable "build_job_hostmounts" { | |||
description = "A map of name:path for which each named path will cause the host path to be mounted at the identical mountpoint in the build container." | |||
default = {} |
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.
THis should be a list of maps
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.
@chickenandpork
this should be
default = []
type = list(map(any))
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.
My intent was to allow the key become the name of the map; this gives a somewhat trivial self-documenting of which rule creates a given hostmount (in case it's not obvious) and -- for me -- allows tying back a config piece to a related feature or function (in my example, "dogstatsd" is the key, and the host mount is used to host mount to get access to the dogstatsd Unix Domain Socket)
If optional types were permitted, I'd use this:
variable "build_job_hostmounts" {
...
default = {}
type = map(object({
host_path = string
container_path = optional(string)
read_only = optional(bool)
}))
}
This allows me to write:
build_job_hostmounts = {
dogstatsd = {
host_path = "/var/run/datadog"
},
some_other_mount = {
host_path = "/some/path"
container_path = "/other/path"
read_only = true
},
}
The dogstatsd
appears in the config.toml as:
[[runners.kubernetes.volumes.host_path]]
name = "dogstatsd"
mount_path = "/var/run/datadog"
host_path = "/var/run/datadog"
read_only = false
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.
that makes more sense. But optional vars I think were added 0.14; so we cannot use that. In that case, add an example in the README of using this and also we'll have to make the description as explanatory as possible.
The description says list, but ideally should be a map of maps. We could set the type as:
type = map(map(string))
although, I'm not sure if that'll work.
Hi @MeNsaaH; thanks again for taking a look. Based on your suggestions,I have :
|
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.
Can you also rebase your branch from master. I fixed the CI
@@ -105,6 +105,11 @@ variable "local_cache_dir" { | |||
description = "Path on nodes for caching" | |||
} | |||
|
|||
variable "build_job_hostmounts" { | |||
description = "A map of name:path for which each named path will cause the host path to be mounted at the identical mountpoint in the build container." | |||
default = {} |
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.
@chickenandpork
this should be
default = []
type = list(map(any))
CI for |
Yes, but the version of I notice that the updated |
e0f4964
to
a28c0d8
Compare
a28c0d8
to
939fb63
Compare
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
@chickenandpork thank you for you contribution |
## [1.5.0](v1.4.0...v1.5.0) (2022-11-10) ### Features * Map hostmount volumes through to config.toml ([#12](#12)) ([8641b32](8641b32))
This PR is included in version 1.5.0 🎉 |
This PR allows the gitlab-runner to have host paths mapped through to the container as
runners.kubernetes.volumes.host_path
blocks.For example, I wanted Datadog UDS (
/var/run/datadog/dsd.socket
) to report metrics:This of course creates the
config.toml
section, with intentionally the same path:Of course this is https://docs.gitlab.com/runner/executors/kubernetes.html#host-path-volumes in the docs.