-
Notifications
You must be signed in to change notification settings - Fork 15
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
Remove MS Remote Desktop support #1535
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.
LGTM this has cleared out a lot of code to support 🎉.
A few references to MSRDS remaining in the smoke tests README but that isn't a big deal, it is just an example SRE name. |
I found a few more in the security checklist, which I'm just fixing along with the linting issues, so I'll fix this one too. |
Oh, these are fine. This is the desktop app for connecting to remote windows machines. |
It's still necessary to use MS Remote Desktop to interact with the domain controllers |
|
This one is worth changing Another reference to "RDS" in the same file. I'm not sure what the JSON files in the test directory or the DC desire state files mean. @jemrobinson? |
Hm, with that one - the |
If I understand, yes I think we can keep the variable and if we have a variable we should check that it is valid. Removing all of this is probably unnecessary work. Also, like you said, it could be useful to prompt people to 'upgrade' and give a useful error message. We can make Guacamole the default though, so that you don't need to specify this in configuration files. |
I suggest making it a non-configurable option (ie. don't check for in in Configuration.psm1 but just set it to "ApacheGuacamole" there). This would remove it from the example configs but leave it in the full configs. |
Co-authored-by: Jim Madge <[email protected]>
Co-authored-by: Jim Madge <[email protected]>
Co-authored-by: Jim Madge <[email protected]>
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.
@JimMadge I think all your comments have been addressed now
✅ Checklist
Enable foobar integration
rather than515 foobar
).develop
.'[WIP]'
to the title if needed (if you're not yet ready to merge)../tests/AutoFormat_Powershell.ps1 -TargetPath <path to file or directory>
for Powershell).Removes support for MS Remote Desktop.
NOTE: do not merge before Release 4.0.4 is complete
🌂 Related issues
Closes #1159
🔬 Tests
Deployed an SHM and Guacamole Tier 2 SRE successfully. Rebuilt docs locally.