-
Notifications
You must be signed in to change notification settings - Fork 849
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-14740] Integrate GPO functionality #961
Conversation
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.
Build tested on 2 machines. Have tested all the combinations. Not tested in a proper AD en though but should work.
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 just need to create a follow up ticket for an unrelated task
@@ -24,6 +24,7 @@ | |||
"semver": "^5.5.0", | |||
"simple-spellchecker": "^0.9.8", | |||
"underscore": "^1.9.1", | |||
"winreg": "^1.2.4", |
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.
can we file a ticket to have all of this dependencies to be exact?
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.
@enahum Why do you need them to be exact? The API shouldn't change for 1.x.x, so ^
appears to be correct IMHO.
https://stackoverflow.com/a/22345808/3514658
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.
Or maybe you want to have ~
instead?
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.
cause we use exact versions for dependencies everywhere and this one should not be the exception ;)
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.
@enahum Ok. Let's open a ticket for this asking to bumb dependencies as well. Because right, now according to npm
there are still some deps we use for which we haven't applied updates. In the meantime, I merge this PR, because bumping dependencies will require another test session I don't want to do now since we have our open test release today :)
removed 1 package and audited 25559 packages in 55s
found 34 vulnerabilities (15 moderate, 19 high)
run `npm audit fix` to fix them, or `npm audit` for details
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.
Yeap, as I said this has to be another ticket which I already created https://mattermost.atlassian.net/browse/MM-15208
* integrate gpo functionality * support multiple windows registry ‘hives’ * correct some copy paste errors * registry config progress * tweaks
Before submitting, please confirm you've
npm run lint:js
for proper code formattingPlease provide the following information:
Summary
This PR integrates GPO functionality by retrieving supported values set in the Windows registry and appropriately merging them with the app config.
NOTE: This functionality needs to be tested in a Windows environment configured with the appropriate GPO capabilities.