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

[breaking change] Add pre-upgrade hook for setup #335

Merged
merged 2 commits into from
Dec 20, 2019

Conversation

maimaisie
Copy link
Contributor

Description

Revert #288 since we have confirmed that upgrading from non-TF chart to TF chart with setup running in pre-upgrade will potentially create a new collector (breaking change).

Will update CHANGELOG in #333.

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@rvmiller89
Copy link
Contributor

Will this cause issues until we get a fix for https://jira.kumoroku.com/jira/browse/SUMO-125138 ?

If their collector name has a dot in it, this would run the job during pre-upgrade, which would fail.

@maimaisie
Copy link
Contributor Author

That's a great point, I'm afraid it would. Do we know the QE timeline? We might still be able to get this in if the URL fix makes to production next week.

@rvmiller89
Copy link
Contributor

Just merged the fix for that, so we just want to upgrade api with that fix before we do the major release with this.

@rvmiller89
Copy link
Contributor

To be safer, could we delay merging this until we know we want to add a new source? I'm concerned there could be other Terraform gotchas that will take some time to discover.

@maimaisie maimaisie changed the title Add pre-upgrade hook for setup [breaking change] Add pre-upgrade hook for setup Dec 17, 2019
@maimaisie
Copy link
Contributor Author

@rvmiller89 I missed your last comment. I was going to merge this PR today, I think it should be fine to merge now since QE will be testing the upgrade flow. What do you think?

@rvmiller89
Copy link
Contributor

I guess if it's already in the QE test plan I'm fine with merging. If customers face issues upgrading we can tell them to remove ,pre-upgrade from their values.yaml

@maimaisie
Copy link
Contributor Author

Sounds good, thanks @rvmiller89

@maimaisie maimaisie merged commit c42ee5c into master Dec 20, 2019
@maimaisie maimaisie deleted the maisie-pre-upgrade-revert branch December 20, 2019 19:15
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.

3 participants