Skip to content
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.

Sign up for GitHub

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

[installer] Add disableDefinitelyGp config option #8308

Merged
merged 1 commit into from
Feb 21, 2022
Merged

[installer] Add disableDefinitelyGp config option #8308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/installer/pkg/components/server/configmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

@mrsimonemms mrsimonemms Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bonus points for checking that definitelyGpDisabled in gitpod.yaml is set to false when disableDefinitelyGp is not set in gitpod.config.yaml file.

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

WorkspaceGarbageCollection: WorkspaceGarbageCollection{
ChunkLimit: 1000,
ContentChunkLimit: 1000,
Expand Down
2 changes: 2 additions & 0 deletions components/installer/pkg/config/v1/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ type Config struct {

SSHGatewayHostKey *ObjectRef `json:"sshGatewayHostKey,omitempty"`

DisableDefinitelyGP bool `json:"disableDefinitelyGp,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

@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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


Experimental *experimental.Config `json:"experimental,omitempty"`
}

Expand Down