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

Jump to bottom

[installer] Support enabling protected secrets #13484

Merged
merged 1 commit into from
Sep 30, 2022
Merged

[installer] Support enabling protected secrets #13484

merged 1 commit into from
Sep 30, 2022

Conversation

csweichel
Copy link
Contributor

Description

Adds support for enabling protected secrets in the installer.

How to test

Run the installer with the following config:

experimental:
  workspace:
    enableProtectedSecrets: true

and observe how the server configmap now contains a new default feature flag.

Release Notes

Add experimental support for protected secrets

@csweichel csweichel requested review from a team September 30, 2022 10:25
@github-actions github-actions bot added team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team labels Sep 30, 2022
@easyCZ
Copy link
Member

easyCZ commented Sep 30, 2022 •
edited
Loading

Currently it changes the rendering of the installer config when no flag is specified to "defaultFeatureFlags": null. Is that intentional? It used to be the empty list []

@Furisto
Copy link
Member

Furisto commented Sep 30, 2022

Not intentional, but also not harmful I think. I will change it anyway, both to keep it consistent and to fix the test failure.

easyCZ
easyCZ approved these changes Sep 30, 2022
View reviewed changes
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

LGTM.

An aside, this is one thing which the ws-director would be able to help us solve - not needing to give DefaultWorkspaceFlags to server and only configure it in the director which is way closer to the problem at hand.

@nandajavarma
Copy link
Contributor

nandajavarma commented Sep 30, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-sh-preview

👍 started the job as gitpod-build-cw-enable-ps-sh.2
(with .werft/ from main)

@atduarte atduarte mentioned this pull request Sep 30, 2022
Closed
12 tasks
nandajavarma
nandajavarma reviewed Sep 30, 2022
View reviewed changes
install/installer/pkg/config/v1/experimental/experimental.go Outdated
@@ -119,6 +119,8 @@ type WorkspaceConfig struct {
// Deprecated
UsageReportBucketName string `json:"usageReportBucketName"`
} `json:"contentService"`

EnableProtectedSecrets bool `json:"enabledProtectedSecrets"`
Copy link
Contributor

@nandajavarma nandajavarma Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: is enabled a typo? Just because in the example you gave in the description it was enable and kinda took me a minute to realize when testing. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a typo. Great catch :)

nandajavarma
nandajavarma approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@nandajavarma nandajavarma left a comment

Choose a reason for hiding this comment

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

Added a nit comment. But works well. Adding a hold for visibility.

/hold

@nandajavarma
Copy link
Contributor

/unold :shipit:

@nandajavarma
Copy link
Contributor

/unhold
(lol I made a typo on unhold)

@roboquat roboquat merged commit 9c7e7c3 into main Sep 30, 2022
@roboquat roboquat deleted the cw/enable-ps-sh branch September 30, 2022 13:07
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Oct 3, 2022
@adrienthebo adrienthebo mentioned this pull request Oct 3, 2022
Merged
3 tasks
@kylos101 kylos101 mentioned this pull request Oct 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nandajavarma nandajavarma nandajavarma approved these changes

@easyCZ easyCZ easyCZ approved these changes

Assignees

@csweichel csweichel

Labels
deployed: webapp Meta team change is running in production priority: high release-note size/S team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team
Projects
No open projects
🌌 Workspace Team
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@csweichel @easyCZ @Furisto @nandajavarma @atduarte @roboquat