Skip to content

Add non-AD desktops to Enroll New Resource#35231

Merged
probakowski merged 15 commits intomasterfrom
probakowski/enroll-non-ad
Dec 12, 2023
Merged

Add non-AD desktops to Enroll New Resource#35231
probakowski merged 15 commits intomasterfrom
probakowski/enroll-non-ad

Conversation

@probakowski
Copy link
Copy Markdown
Contributor

@probakowski probakowski commented Nov 30, 2023

This change add new tile to Enroll New Resource for non-AD Windows Desktops.
It's unguided one, just providing link to the guide.

Zrzut ekranu 2023-12-6 o 18 09 30

This PR depends on https://github.com/gravitational/cloud/pull/6927

Changelog: Add non-AD desktops to Enroll New Resource

@probakowski probakowski requested a review from zmb3 November 30, 2023 20:54
@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but the event field needs to be adjusted AFAIK.

ApplicationHttp = 'DISCOVER_RESOURCE_APPLICATION_HTTP',
ApplicationTcp = 'DISCOVER_RESOURCE_APPLICATION_TCP',
WindowsDesktop = 'DISCOVER_RESOURCE_WINDOWS_DESKTOP',
WindowsDesktopNonAD = 'DISCOVER_RESOURCE_DOC_WINDOWS_DESKTOP_NON_AD',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to correspond to DiscoverResource in usageevents.proto which itself needs to be deployed to Cloud first.

// DiscoverResource represents a resource type.
enum DiscoverResource {
DISCOVER_RESOURCE_UNSPECIFIED = 0;
DISCOVER_RESOURCE_SERVER = 1;
DISCOVER_RESOURCE_KUBERNETES = 2;
DISCOVER_RESOURCE_DATABASE_POSTGRES_SELF_HOSTED = 3;
DISCOVER_RESOURCE_DATABASE_MYSQL_SELF_HOSTED = 4;
DISCOVER_RESOURCE_DATABASE_MONGODB_SELF_HOSTED = 5;
DISCOVER_RESOURCE_DATABASE_POSTGRES_RDS = 6;
DISCOVER_RESOURCE_DATABASE_MYSQL_RDS = 7;
DISCOVER_RESOURCE_APPLICATION_HTTP = 8;
DISCOVER_RESOURCE_APPLICATION_TCP = 9;
DISCOVER_RESOURCE_WINDOWS_DESKTOP = 10;

For now I'd just use WindowsDesktop and ask on #dev-discover if we want to track these separately, unless it was discussed already.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. We absolutely want to track separately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that, I've modified the proto and opened PR in cloud

Copy link
Copy Markdown
Contributor

@ibeckermayer ibeckermayer Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zmb3 @probakowski wdyt about adding a usage event for reaching the end of the discover AD flow as well (per our discussion in the call last week).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest the same. Might as well since we're here.

@probakowski probakowski force-pushed the probakowski/enroll-non-ad branch from 72f59da to ce4a338 Compare December 6, 2023 17:11
Copy link
Copy Markdown
Contributor

@ibeckermayer ibeckermayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See suggestions

Comment thread web/packages/teleport/src/Discover/SelectResource/resources.tsx Outdated
Comment thread web/packages/teleport/src/Discover/SelectResource/resources.tsx Outdated
@probakowski probakowski added this pull request to the merge queue Dec 11, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Dec 11, 2023
@probakowski probakowski added this pull request to the merge queue Dec 11, 2023
Merged via the queue into master with commit aa25bfb Dec 12, 2023
@probakowski probakowski deleted the probakowski/enroll-non-ad branch December 12, 2023 00:07
@public-teleport-github-review-bot
Copy link
Copy Markdown

@probakowski See the table below for backport results.

Branch Result
branch/v14 Failed

probakowski added a commit that referenced this pull request Dec 15, 2023
* Add non-AD to Enroll new resource

* add discoverresource to proto

* reword desktops labels

* Update UI test snapshot

* Update UI test snapshot

* Update web/packages/teleport/src/Discover/SelectResource/resources.tsx

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* Update web/packages/teleport/src/Discover/SelectResource/resources.tsx

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* Update UI test snapshot

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
github-merge-queue Bot pushed a commit that referenced this pull request Dec 18, 2023
* Add non-AD desktops to `Enroll New Resource` (#35231)

* Add non-AD to Enroll new resource

* add discoverresource to proto

* reword desktops labels

* Update UI test snapshot

* Update UI test snapshot

* Update web/packages/teleport/src/Discover/SelectResource/resources.tsx

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* Update web/packages/teleport/src/Discover/SelectResource/resources.tsx

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* Update UI test snapshot

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* fix test

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants