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

Revert "Make puma default webserver (#481)" #483

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

Samze
Copy link
Contributor

@Samze Samze commented Oct 24, 2024

This reverts commit 781690e.


While we would like to see Puma be made the default web server, removing this property with no notice is problematic for us as we have non cf-d deployments that consume this property explicitly. Instead, perhaps for now these property defaults could be changed to true for a period of releases before being removed in its entirety.

We'd also like to workout a more formal deprecation policy for properties. Additionally, going forward, I think this pattern of including experimental in the name of the property only causes problems when we have to eventually remove them and replace them with something else. Perhaps we could have just had enable_puma as a property with a textual description stating it was experimental and then eventually remove that phrasing. We can discuss this on the WG call.

@Samze and @sethboyles

  • I have viewed signed and have submitted the Contributor License Agreement

  • I have made this pull request to the develop branch

  • I have run CF Acceptance Tests on bosh lite

@Samze
Copy link
Contributor Author

Samze commented Oct 24, 2024

cc @svkrieger / @jochenehret

@Gerg
Copy link
Member

Gerg commented Oct 24, 2024

I think it's also useful to have the property around for at least a few releases as an "escape valve" in case operators run into issues after upgrading to capi-release versions that default to Puma.

@philippthun
Copy link
Member

My proposal for going forward:

  1. We keep cc.experimental.use_puma_webserver, but
    (a) move it to a newly introduced "deprecated" section in the spec file.
    (b) write about this deprecation in the release notes of the next CAPI release.
    (c) remove it in 6 months (we don't have the notion of major releases for CAPI, so this is the best I could think of).
    (d) remove the default value.

  2. We introduce cc.temporary_enable_deprecated_thin_webserver with default false.

The coding to determine which webserver to use would look like this:

if_p('cc.experimental.use_puma_webserver') do |prop|
 webserver = prop ? 'puma' : 'thin'
else
 webserver = p('cc.temporary_enable_deprecated_thin_webserver') ? 'thin' : 'puma'
end

A table of config combinations showing that the deprecated config option takes precedence:

cc.temporary_enable_deprecated_thin_webserver cc.experimental.use_puma_webserver webserver
false (unset) puma
false false thin
false true puma
true (unset) thin
true false thin
true true puma

So we change the default webserver from thin to puma, but we allow operators to either use the old or the new config option to switch back to thin.

@svkrieger
Copy link
Contributor

Philipp's suggestion sounds reasonable to me. No config change needed for your deployments for now and we only have to remove cc.experimental.use_puma_webserver in the future to clean up the possible configurations.

I'll merge the revert PR for now. Let me know what ya'll think, then I'll incorporate the changes.

@svkrieger svkrieger merged commit f39231c into develop Oct 25, 2024
2 checks passed
@svkrieger svkrieger deleted the revert_puma_property_change branch October 25, 2024 12:41
@stephanme
Copy link

+1 for Philipps proposal.

I put the topic "Deprecation policy for Bosh configuration properties" on the agenda of the next CAPI Open Office Hour.
Maybe we find a simpler solution to introduce new experimental features, make it default and eventually remove the old solution.

@svkrieger svkrieger mentioned this pull request Oct 29, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants