-
Notifications
You must be signed in to change notification settings - Fork 58
feat: add module for Web RDP #262
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
Conversation
Co-authored-by: Asher <ash@coder.com>
/** | ||
* Finds the only "coder_script" resource in the given state and runs it in a | ||
* container. | ||
*/ |
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.
Updated the comment formatting so that these would be public, and would show up when someone hovers over the function definition
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.
TIL: /**
are public comments and //
are private 🤦
]; | ||
}; | ||
|
||
resources: [TerraformStateResource, ...TerraformStateResource[]]; |
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.
Redefined this so that resources
is an array with at least element, rather than an array with exactly one element. This typo was causing some of the other tests to do stuff with the any
type; I'll be making a separate PR to clean up more stuff
* Creates a test-case for each variable provided and ensures that the apply | ||
* fails without it. | ||
*/ | ||
export const testRequiredVariables = <TVars extends Record<string, string>>( |
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.
Added a type parameter so that you can define what properties you expect to be valid for the function, so that you can more easily catch accidental typos
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 👍 smoke-tested
* don't overshoot and grab too much content. | ||
* | ||
* Written and tested via Regex101 | ||
* @see {@link https://regex101.com/r/UMgQpv/2} |
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.
👍
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.
The JS code looks good, but since I don't have extensive context on this, I'm unsure if I'm the right person to approve it. Also, how should we conduct QA for this PR?
Edit: I was able to partially QA it, and it seems fine. It would be helpful to have instructions in the description on how to QA this feature.
@BrunoQuaresma So, since it's a module, there's kind of two main ways to do it:
Edit: The above is for Google Cloud. We also have an AWS template. |
slug = "web-rdp" | ||
display_name = "Web RDP" | ||
url = "http://localhost:7171" | ||
icon = "https://svgur.com/i/158F.svg" |
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.
Icon path needs to be updated.
agent_id = var.agent_id | ||
display_name = "Local RDP" | ||
slug = "rdp-docs" | ||
icon = "https://raw.githubusercontent.com/matifali/logos/main/windows.svg" |
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.
icon needs to be added to coder icons
function Install-DevolutionsGateway { | ||
# Define the module name and version | ||
$moduleName = "DevolutionsGateway" | ||
$moduleVersion = "2024.1.5" |
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.
should the version be exposed as a variable to allow users to easily upgrade/downgrade Devolutions gateway? Its fine as it is too if we like to tightly control it ourselves to preserver compatibility and ensure everything works as tested.
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.
Yeah, just because the DOM selectors in the JavaScript code are so specific, it felt really important to pin Devolutions Gateway to a specific version, to avoid breaking changes as the Devolutions team refactors their own UI
It would be nice to expose it as a mutable value, but I feel like it might be asking for trouble, and could cause things to break unexpectedly
@Parkreiner you can merge the PR and then we can cut a release which should make it available on registry.coder.com |
Closes #234
Changes made
Notes