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

Release 3.2.5 #937

Merged
merged 6 commits into from
May 8, 2023
Merged

Release 3.2.5 #937

merged 6 commits into from
May 8, 2023

Conversation

trilopin
Copy link
Contributor

@trilopin trilopin commented May 3, 2023

Summary

Fixes

  • Telemetry race condition
  • Telemetry rudder keys

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-52608

After some name changing, MM_RUDDER_WRITE_KEY has become MM_RUDDER_PLUGINS_PROD.
Fixes hook race condition for telemetry (onactivate, onconfigchanged)

mattermost-server and pluginapi deps have been updated, opengraph forced as an indirect dep to avoid a ambiguous dep conflict
@trilopin trilopin added the Work In Progress Not yet ready for review label May 3, 2023
trilopin and others added 2 commits May 3, 2023 12:02
Configure telemetry fixes locally to avoid increasing min_server_version (rollbacked to 6.5.2)

This fixes the problems introduced by #930.

This locally copied telemetry package can be removed in favor of [email protected] when we can safely upgrade min_server_version to 7.4.

Fixes https://mattermost.atlassian.net/browse/MM-51548
@@ -539,7 +539,6 @@ func (p *Plugin) httpGetJiraProjectMetadata(w http.ResponseWriter, r *http.Reque
}
}

type option = utils.ReactSelectOption
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this in addition to the cherry-pick because the linter complained.

@trilopin trilopin marked this pull request as ready for review May 3, 2023 10:58
@trilopin trilopin requested a review from mickmister as a code owner May 3, 2023 10:58
@trilopin trilopin requested review from DHaussermann and srkgupta May 3, 2023 10:58
@trilopin trilopin added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed Work In Progress Not yet ready for review labels May 3, 2023
@trilopin trilopin added this to the v3.2.5 milestone May 3, 2023
@@ -2625,7 +2625,7 @@
"amdefine": {
"version": "1.0.1",
"resolved": "https://registry.npmjs.org/amdefine/-/amdefine-1.0.1.tgz",
"integrity": "sha1-SlKCrBZHKek2Gbz9OtFR+BfOkfU=",
"integrity": "sha512-S2Hw0TtNkMJhIabBwIojKL9YHO5T0n5eNqWJ7Lrlel/zDbftQpxpapi8tZs3X1HWa+u+QeydGmzzNU0m09+Rcg==",
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental changes? Maybe part of cherrypick though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran full make deploy after the merge to ensure everything works together, and npm never misses the opportunity to make some changes.

about integrity hashes, should we ignore that?

Copy link
Contributor

Choose a reason for hiding this comment

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

about integrity hashes, should we ignore that?

I'm honestly not sure. @hmhealey Do you have any thoughts on this? Using sha512 is certainly more thorough than sha1. I'm mainly wondering if we should converge on the sha512 hashes all around to make sure unintentional lock file changes like this are avoided. (Sorry I always mention you for npm questions/conventions)

Copy link
Member

Choose a reason for hiding this comment

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

No worries on the mention since I've spent so much time trying to understand the chaos of npm 😛

As long as they're not changing back and forth, I think we can be fine with whatever. Based on this, npm's been using sha512 for a few years now, and the web app's package-lock has all sha512 hashes as well. I'd assume those previous hashes were from an older version of npm

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

  • Looked over the branch and only telemetry changes are implemented

  • No functional testing needed for any changes form my side

  • Quick smoke test as a precaution

    • Installed instance
    • Connect / disconnect
    • Create an attach issue work
    • Spot check slash commands
    • Issue subscriptions are functional
    • As precaution, ensure the build from the pipeline starts and runs on a cloud server

    LGTM! thanks @trilopin

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 8, 2023
@trilopin trilopin merged commit acca43d into release-3.2 May 8, 2023
@trilopin trilopin deleted the release-3.2.5 branch May 8, 2023 07:22
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants