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

MM-23227: preserve disable across config changes #93

Merged
merged 4 commits into from
Apr 2, 2020

Conversation

lieut-data
Copy link
Member

Summary

Fix an issue with how the configuration structure was always being reinitialized whenever a system configuration change occurred. Also avoid mutating the configuration in place.

Note that the go mod tidy commit was run on vanilla master, not specifically my changes. Not sure how that slipped in.

Ticket Link

https://mattermost.atlassian.net/browse/MM-23227

Fix an issue with how the configuration structure was always being reinitialized whenever a system configuration change occurred. Also avoid mutating the configuration in place.

https://mattermost.atlassian.net/browse/MM-23227
@lieut-data lieut-data added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 12, 2020
Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

LGTM! Looks like the CI is complaining about the coverage, but I don't understand why.

@lieut-data
Copy link
Member Author

Oops, yeah -- looks like the tests were actually failing. Fixed!

@codecov-io
Copy link

Codecov Report

Merging #93 into master will increase coverage by 1.4%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #93     +/-   ##
=========================================
+ Coverage   16.24%   17.64%   +1.4%     
=========================================
  Files          13       13             
  Lines        1139     1139             
=========================================
+ Hits          185      201     +16     
+ Misses        916      900     -16     
  Partials       38       38
Impacted Files Coverage Δ
server/command_hooks.go 8.72% <0%> (ø) ⬆️
server/configuration.go 51.33% <100%> (+10.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24dbd65...249ca2d. Read the comment docs.

Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

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

LGTM!

@jfrerich jfrerich removed the 2: Dev Review Requires review by a core committer label Mar 19, 2020
@jfrerich jfrerich requested a review from DHaussermann March 19, 2020 02:58
@hanzei hanzei added this to the v0.6.0 milestone Mar 19, 2020
@hanzei hanzei mentioned this pull request Apr 2, 2020
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed
I used the the user login event to be able to see when hooks are in fact disabled and confirmed I could repro the issue with previous release.

Tested and ensured that hooks now remain disabled when a new change is saved to the config.
Add a release test to cover this issue.
LGTM! Thanks @lieut-data for QA steps on this one.

Please merge.

@DHaussermann DHaussermann removed the 3: QA Review Requires review by a QA tester label Apr 2, 2020
@lieut-data lieut-data merged commit e7785b9 into master Apr 2, 2020
@lieut-data lieut-data deleted the mm-23227-preserve-disable-across-config-changes branch April 2, 2020 19:56
@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA labels Apr 2, 2020
@hanzei hanzei mentioned this pull request Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants