sighupio / furyagent

Apache License 2.0
9 stars 2 forks source link

added privileged feature flag parameter #24

Open lzecca78 opened 3 years ago

lzecca78 commented 3 years ago

As requested, this PR is to add the feature flag for privileged mode on user creation when ssh_user flag is invoked

angelbarrera92 commented 3 years ago

I see the new attribute is a string instead of a boolean.

I now the reason why you develop it using string (the default value in boolean is false) but clients can be impacted:

privileged: true

This will not work with your approach

vs

This works

privileged: "true"

What do you think? @phisco @ralgozino ???

ralgozino commented 3 years ago

I see the new attribute is a string instead of a boolean.

I now the reason why you develop it using string (the default value in boolean is false) but clients can be impacted:

privileged: true

This will not work with your approach

vs

This works

privileged: "true"

What do you think? @phisco @ralgozino ???

that's a fair point. If the issue is actually with booleans having false as zero value in golang, we could change the config parameter from privileged to unprivileged or something like that, i.e. negate the option instead of the value.

angelbarrera92 commented 3 years ago

I see the new attribute is a string instead of a boolean. I now the reason why you develop it using string (the default value in boolean is false) but clients can be impacted:

privileged: true

This will not work with your approach vs This works

privileged: "true"

What do you think? @phisco @ralgozino ???

that's a fair point. If the issue is actually with booleans having false as zero value in golang, we could change the config parameter from privileged to unprivileged or something like that, i.e. negate the option instead of the value.

Ye, I thought about it too, but we wanted to keep as close to k8s terminology as possible. I found a "non-clean"/"dirty" way to do exactly what we need: https://github.com/go-yaml/yaml/issues/165#issuecomment-255223956

lzecca78 commented 3 years ago

@ralgozino that's the point why i shifted to string variable instead boolean. I didn't like the negation attribute value unprivileged BUT i prefer by far the negation attribute instead of wrapping the unmarsheller to set the default value, that is awful. Put this choice by vote: ❤️ : if you prefer the leave the string value as is with privileged attribute ⭐ : if you prefer to rename the variable to unprivileged converting it to boolean

I neither add the third option because is immoral and against the law (wrap the unmarshal function)

ralgozino commented 3 years ago

❤️ : if you prefer the leave the string value as is with privileged attribute ⭐ : if you prefer to rename the variable to unprivileged converting it to boolean

(I can't react with a star, so I'm commenting)