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.

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]: promote proxy service type from experimental #11006

Merged
merged 5 commits into from
Jul 6, 2022

Conversation

mrsimonemms
Copy link
Contributor

@mrsimonemms mrsimonemms commented Jun 29, 2022

Anyone relying upon this config in the experimental section should move to the components. See description for new format

Description

Deprecated the proxy's service type in experimental and promotes it to the primary configuration. Implements the configuration as per #10983.

Includes the KOTS config section in a separate "Components" group - the reasoning behind this is that there may be additional component-specific configuration in the future. This should make it easier to manage/visualise. This is only displayed if the "enable additional options" is enabled.

Adds the ability to deprecate config parameters.

I've also renamed "additional options" to "advanced options"

image

Related Issue(s)

Fixes #11008
Fixes #11009
Fixes #11025
Fixes #11033

How to test

Release Notes

[installer]: promote proxy service type from experimental

Documentation

Werft options:

  • /werft with-preview

@mrsimonemms
Copy link
Contributor Author

mrsimonemms commented Jun 29, 2022

/werft run no-preview publish-to-kots

👍 started the job as gitpod-build-sje-installer-clusterip.2
(with .werft/ from main)

@mrsimonemms mrsimonemms marked this pull request as ready for review June 29, 2022 14:43
@mrsimonemms mrsimonemms requested review from a team June 29, 2022 14:43
@github-actions github-actions bot added team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team labels Jun 29, 2022
@roboquat roboquat added size/L and removed size/M labels Jun 30, 2022
@geropl geropl self-assigned this Jun 30, 2022
@mrsimonemms mrsimonemms marked this pull request as draft July 4, 2022 11:29
@mrsimonemms mrsimonemms force-pushed the sje/installer-clusterip branch 3 times, most recently from 84c0398 to cfc9446 Compare July 4, 2022 14:41
@mrsimonemms
Copy link
Contributor Author

Based upon @geropl's feedback, this PR has introduced the ability to deprecate fields from the Installer config which prints a deprecation notice to stderr and have added the experimental.webapp.proxy.serviceType param to it

Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

I have a comment regarding overwriting the experimental config with the new config together with the KOTS UI change. I think that could break the installation when users have a config patch that set the serviceProxy in the experimental section because this will be overwritten by the new proxy config in any case (solely by updating Gitpod with KOTS). I think we need at least a warning, better a property way to handle this.

The remaining comments are my 2 cents only. Feel free to disagree and ignore.

@mrsimonemms
Copy link
Contributor Author

Ok, I've updated this so it has @corneliusludmann's elegant proposal of dealing with the experimental values.

I've also changed the KOTS config so that the LoadBalancer option doesn't actually set any value. This is so that it doesn't silently break the workflow for anyone upgrading who is using the experimental version already.

I want to refactor how the KOTS config is generated with the config patch file. Deprecations will come in that

@mrsimonemms mrsimonemms marked this pull request as ready for review July 5, 2022 12:24
Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

Love it. 🤩

Thanks for taking my suggestions into consideration. 🙏

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Nice pattern, thank you @mrsimonemms !

@roboquat roboquat merged commit 1b1248d into main Jul 6, 2022
@roboquat roboquat deleted the sje/installer-clusterip branch July 6, 2022 11:28
@mrsimonemms
Copy link
Contributor Author

Team effort - couldn't have done it without you two

Eiffel Tower

@roboquat roboquat added the deployed: webapp Meta team change is running in production label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production release-note size/L team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team
Projects
None yet
4 participants