-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Chore: Update Meteor to 2.5.3 #24075
Conversation
It apparently is compatiable with IE11 again meteor/meteor#11772
so looks like the async hooks issue was fixed on recent node versions nodejs/node#34556 (v15+) |
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.
You have added 6 .js files, please convert to ts(x).
packages/accounts-linkedin/linkedin.js
packages/accounts-linkedin/notice.js
packages/accounts-linkedin/package.js
packages/linkedin-oauth/linkedin-client.js
packages/linkedin-oauth/linkedin-server.js
packages/linkedin-oauth/package.js
with all package dependencies updated, everyone is able to run the code now. |
This pull request introduces 7 alerts when merging ad5f566 into bf6878e - view on LGTM.com new alerts:
|
This pull request introduces 1 alert and fixes 2 when merging b665425 into 0e755f2 - view on LGTM.com new alerts:
fixed alerts:
|
@RocketChat/documentation-team fyi - parts of our documentation might need update. Would be good to have them ready before this gets merged, especially with the gsoc students coming in. Let me know if I can help in any way. |
cc @Rodriq (since you weren't in the gh team when i mentioned by that) |
This pull request introduces 1 alert and fixes 2 when merging 1df2cb4 into 50d55d7 - view on LGTM.com new alerts:
fixed alerts:
|
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.
Great job
@@ -230,7 +230,7 @@ | |||
"express": "^4.17.1", | |||
"express-rate-limit": "^5.2.6", | |||
"fflate": "^0.7.1", | |||
"fibers": "4.0.3", | |||
"fibers": "^5.0.0", |
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.
Is this going to break CentOS 7 support?
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.
I hope not. I can make sure and test it though
This pull request introduces 1 alert and fixes 2 when merging 1544a5e into a06e811 - view on LGTM.com new alerts:
fixed alerts:
|
af58958
This pull request introduces 1 alert and fixes 2 when merging af58958 into a06e811 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 2 when merging 4644f91 into f37c2f4 - view on LGTM.com new alerts:
fixed alerts:
|
@@ -36,12 +36,12 @@ RUN aptMark="$(apt-mark showmanual)" \ | |||
&& apt-mark auto '.*' > /dev/null \ | |||
&& apt-mark manual $aptMark > /dev/null \ | |||
&& find /usr/local -type f -executable -exec ldd '{}' ';' \ |
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.
&& find /usr/local -type f -executable -exec ldd '{}' ';' \ | |
&& find /usr/local -type f -perm /111 -exec ldd '{}' ';' \ |
-executable
doesn't exactly detect if a file is executable or not, since it uses the access
syscall. It can potentially return all files (and dirs if no -type f
) for a given directory. Since we are updating a lot of stuff in this pr already, let's also change it from this point.
I have not tested it yet, but this change shouldn't affect the image in any way.
Let me know if I'm misreading the purpose of this line 🙈
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.
"on behalf of" definitely not right 😬
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.
I have to say I don't know what that does 😅 it was copied from node's Dockerfile 🤷
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.
Looks to me like it's making sure all npm/node dependencies are not accidentally removed when removing some other package with a similar dependency, thus not reducing/eliminating child image issues.
Also, looking at the Dockerfile, I'm not sure we even need those lines to be part of our image, do you remember why those were added initially? 🤔
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.
this image specifically is not intended for production, we call is as "preview" since it has mongo installed and can be used to have a "preview" of a rocket.chat version.
we use the same technique on the production one though.. the ideia is to install some packages just to run the npm install
and then clean it up..
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.
Hmm, looks like I commented on the wrong file, but these are part of our production one as well .docker/Dockerfile
.
this isn't a big deal 🙈 maybe slightly more time spent on 'building', nothing else.
This pull request introduces 1 alert and fixes 2 when merging 6a46e57 into 1441feb - view on LGTM.com new alerts:
fixed alerts:
|
Since it is not part of the diff, the Dockerfile contains the If we need them to be part of the Dockerfile for some external tool parsing, we can just mark them without putting any values
Ex. Dockerfile
AFAIK the connection string also has to have the replicaset information a part as well now, which means another assumption by the image. |
…ove/setup-wizard * 'develop' of github.com:RocketChat/Rocket.Chat: (176 commits) [IMPROVE] Admin page header buttons consistency (#24168) i18n: Language update from LingoHub 🤖 on 2022-01-17Z (#24193) [FIX] Integration section crashing opening in My Account (#24068) [IMPROVE] Rewrite roomNotFound to React Component (#24044) Regression: Enable custom emoji on admin custom status page (#24186) Chore: Update Meteor to 2.5.3 (#24075) [NEW] Apple Login (#24060) Chore: Update Apps-Engine to 1.29.2 (#24171) feat: enabling emoji on custom status (#24170) [FIX] App Framework Enable hanging indefinitely (#24158) [FIX] CSV Importer failing to import users (#24090) Fix Engagement Dashboard API requests (#24142) Language update from LingoHub 🤖 (#24127) Chore: Migrate useOutsideClick to fuselage-hooks (#24133) Revert "Use fibers to store context" Use fibers to store context Chore: Include REG_TOKEN in docker-compose (#24123) [FIX] Custom Emoji Image preview #24117 [IMPROVE] Added a Reset Button in the Account Profile Page (#24078) Revert: "[IMPROVE] Throw 404 error in invalid endpoints" (#24118) ...
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
This is a very early draft. Things still missing:Publish updated forkof Meteor packagepauli:linkedin-oauth
- instead of publishing a fork I added the code to/packages
folderPublish updated forkof Meteor packagepauli:accounts-linkedin
- instead of publishing a fork I added the code to/packages
foldernimble:restivus
- published asrocketchat:restivus
rocketchat:oauth2-server
rocketchat:tap-i18n
async_hooks
- we found out Meteor 2.5 is not compatible with Async Hooks (as per Usage of async_hooks / AsyncLocalStorage conflicts with Meteor 2.3+ / Node 12+ meteor/meteor#11539) so we switched to use Fibers insteadrequest
fromHTTPInternals
logoutOtherClients
was removednpmRequestOptions
is not supported anymoreMAIL_URL
set, this changed in recent meteor versions, we need to figure out a way to be able to run tests without an email serviceI have a local copy of all those packages updated, it is just a matter of forking and publishing.The other issues are real work that needs to be done.What needs to be tested
Some parts of the system had to be re-written, so it would be good to have a focus on tests:
console.log