[installer] Add disableDefinitelyGp config option #8308
Conversation
/werft no-preview
github-actions
bot
added
team: delivery
Codecov Report
@@ Coverage Diff @@
## main #8308 +/- ##
========================================
- Coverage 8.42% 7.60% -0.83%
========================================
Files 33 31 -2
Lines 2339 2171 -168
========================================
- Hits 197 165 -32
+ Misses 2137 2003 -134
+ Partials 5 3 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -45,7 +45,7 @@ func configmap(ctx *common.RenderContext) ([]runtime.Object, error) { | |||
| MaxAgeMs: 259200000, | |||
| Secret: "Important!Really-Change-This-Key!", // todo(sje): how best to do this? | |||
| }, | |||
| DefinitelyGpDisabled: false, | |||
| DefinitelyGpDisabled: ctx.Config.DisableDefinitelyGP, | |||
There was a problem hiding this comment.
What happens if this value is not provided in the config? I think we should validate the config and put a default value of false in
There was a problem hiding this comment.
Bonus points for checking that
definitelyGpDisabledingitpod.yamlis set tofalsewhendisableDefinitelyGpis not set ingitpod.config.yamlfile.
false is the default value for bool values in Go. When the value is not given, ctx.Config.DisableDefinitelyGP is false.
The more interesting question is what happens if you don't specify a bool value in your config. However, since the type is bool providing something like disableDefinitelyGp: fake would lead to:
Error: error loading config: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go struct field Config.disableDefinitelyGp of type bool
Is this good enough?
There was a problem hiding this comment.
Yup, I'm happy with that.
It's probably worth setting the tags as json:"disableDefinitelyGp,omitempty" validate:"boolean" to give a bit of protection to avoid someone putting in fake etc
| @@ -88,6 +88,8 @@ type Config struct { | |||
|
|
|||
| SSHGatewayHostKey *ObjectRef `json:"sshGatewayHostKey,omitempty"` | |||
|
|
|||
| DisableDefinitelyGP bool `json:"disableDefinitelyGp,omitempty"` | |||
There was a problem hiding this comment.
@corneliusludmann Would it make sense to start this out in Experimental? Like having an incubation mode of sorts.
Not particular to this flag, but I fear that we start cluttering the config surface again. And I wonder how many installations make use of this flag...?
Alternatively, we could have sth like NoOutboundRequests or AirGappedMode or so that could map to the internal `disableDefinitelyGp? 🤔
There was a problem hiding this comment.
Glad you ask. Very important points.
Regarding experimental
I don't see this as an experimental flag because this is needed by customers already. Experimental config values are more values that we use internally but don't expect that users need this. We don't want to tell customers to use experimental flags when they want to install Gitpod in air-gap mode.
Regarding an airgap flag
I personally would also prefer an airgap flag because disableDefinitelyGp is pretty technical and for end-users not very clear. As far as I know, @csweichel raised the objection regarding an airgap flag that it develops to a magic umbrella flag where no one knows what's behind (maybe he would like to elaborate on this). However ...
installer vs. Replicated
... since we would like customers to move to Replicated eventually, they shouldn't have much to do with the installer config. That means we will set the config that is needed for air-gap installations automatically when users run an air-gap installation with Replicated. The high-level user config surface moves to Replicated and it is ok to have more technical flags in the config.
But again, I think it's important to think about it and really appreciate your thoughts on this. Happy to hear other thoughts as well.
There was a problem hiding this comment.
The high-level user config surface moves to Replicated and it is ok to have more technical flags in the config.
Oh, that's complete news to me, thx for sharing! I always thought the installer config is what we're optimizing for.
There was a problem hiding this comment.
Code change itself LGTM
/hold
In case we want to respond to this comment in this PR.
Description
This change allows disabling definitelyGP with the installer. Useful for air-gap installations.
Related Issue(s)
Fixes #8222
How to test
Add
disableDefinitelyGp: truetogitpod.config.yamlSearch for
definitelyGpDisabledingitpod.yamland make sure it is set totrue.Bonus points for checking that
definitelyGpDisabledingitpod.yamlis set tofalsewhendisableDefinitelyGpis not set ingitpod.config.yamlfile.Release Notes
Meta
/werft no-preview