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

Upgrade to nginx-ingress-controller 0.28.0 #24

Merged
merged 2 commits into from
Jan 30, 2020
Merged

Conversation

sslavic
Copy link

@sslavic sslavic commented Jan 29, 2020

@sslavic sslavic requested review from a team January 29, 2020 22:12
@sslavic sslavic self-assigned this Jan 29, 2020
Copy link
Contributor

@pipo02mix pipo02mix left a comment

Choose a reason for hiding this comment

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

Another option is to create 1.3.0 pointing to this new PR, so we dont have 2 PRs for same version, WDYT?

CHANGELOG.md Outdated
@@ -5,12 +5,12 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project's packages adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).


## [v1.2.1] 2020-01-30
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## [v1.2.1] 2020-01-30
## [v1.3.0] 2020-01-30

If we upgrade a minor version of the app, I would do the same in our chart, dont you think so?

Copy link
Author

@sslavic sslavic Jan 30, 2020

Choose a reason for hiding this comment

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

Sure, versions are cheap. I'd actually prefer not to have app release semantic versions at all (including platform releases) just continuously improved supported managed service. None of these changes in nginx IC 1.3.0 App are breaking backward compatibility from customer perspective so no ack from customer should be required to deploy them, as long as we do it safely. Breaking backward compatibility for a service, is a new service - these should be avoided at all costs, if still done there should be migration path if possible, but surely phase out of no longer supported version. Btw, this is not widely accepted view, just my opinion. From what I learned there will be effort invested by Ludacris into supporting continuous deployment of patch changes (hopefully it covers optional apps too) - I guess under patch it will be considered any change that's not breaking backward compatibility of the API from customer perspective, so again these 1.3.0 changes might be considered then patch level changes, but now it actually doesn't make a difference - major, minor, patch - all go through same platform release process and deployment (cluster upgrade) requires ack from or at least notification to customer.

Anyway, changed it and the release link to point to future 1.3.0 release. These release version links don't link to PR, since there are multiple included in same release.

Copy link
Contributor

@pipo02mix pipo02mix left a comment

Choose a reason for hiding this comment

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

LGTM (we need to update release too after merging)

@sslavic sslavic marked this pull request as ready for review January 30, 2020 11:35
@sslavic
Copy link
Author

sslavic commented Jan 30, 2020

I was hoping to include enabling HPA by default too in the next nginx IC App release, not because I like to cram release (and make it unsafe to deploy, incur testing costs) on contrary would like to ship every change as soon as it passes all automated pre-pord and prod checks - cramming more stuff in each release is as natural reaction to slow cumbersome platform release and delivery process, full of manual human coordination.

Enabling HPA and tuning resources will have to be done in a separate release, since I'm still performing trials to find the right settings and way of configuring them, specifically dealing with variance in tenant clusters, some of the options being

  • configure static resource requests/limits by needs of largest tenant cluster across all customers, or
  • configure static resource requests/limits by needs of smallest tenant cluster across all customers but then manually configure user overrides on every TC itself, or
  • find a way to persist tenant cluster specific static configuration in out infra code, and another alternative being
  • some way of dynamic VPA-like resource adjustments.

@sslavic sslavic merged commit 6e97775 into master Jan 30, 2020
@sslavic sslavic deleted the upgrade-to-v0.28.0 branch January 30, 2020 13:00
@sslavic
Copy link
Author

sslavic commented Jan 30, 2020

@pipo02mix but you released 1.2.1 - didn't know that. Will fix changelog for 1.3.0

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.

2 participants