-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Centralize the GetPackagePath #27004
Conversation
As part of the change to [eliminate MM_SERVER_PATH](mattermost/enterprise#1676), I thought I had properly tested plugins relying on the server for tests, but apparently not, as these tests were now failing trying to resolve the `i18n` directory. To remedy this, and address feedback from the first PR, I've centralized a single `path.go` in the `server` root from which all other paths are built. The return value from this also gets added to the search paths for resolving asset directories at runtime. (Like `MM_SERVER_PATH` did before, but now unconditionally.)
e8e5ced
to
556e7b3
Compare
/e2e-test |
E2E test triggered successfully for PR #27004, commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 One thing that comes to mind that I should have included in the previous review... the GetPackagePath
implementation may break if the place it is called from gets inlined. I think it's possible the result will be wrong if the inlined function is called from a different package.
Good call out, @wiggin77! I'll keep this in mind if we see unexpected breakage in the future. |
Summary
As part of the change to eliminate MM_SERVER_PATH, I thought I had properly tested plugins relying on the server for tests, but apparently not, as these tests were now failing trying to resolve the
i18n
directory.To remedy this, and address feedback from the first PR, I've centralized a single
path.go
in theserver
root from which all other paths are built. The return value from this also gets added to the search paths for resolving asset directories at runtime. (LikeMM_SERVER_PATH
did before, but now unconditionally.)Tested this time with the MS Teams plugin and can confirm the before and after effect of this PR.
Ticket Link
None.
Release Note