Skip to content

Use the examples directory for example plugin code#23255

Merged
ptgott merged 15 commits intomasterfrom
paul.gottschling/2023-03-17-access-plugin
Jul 14, 2023
Merged

Use the examples directory for example plugin code#23255
ptgott merged 15 commits intomasterfrom
paul.gottschling/2023-03-17-access-plugin

Conversation

@ptgott
Copy link
Copy Markdown
Contributor

@ptgott ptgott commented Mar 17, 2023

Closes #27696

Also edit the Access Request plugin API guide to use this directory, rather than having the reader copy/paste individual code snippets. This makes the guide easier to follow, and users will have a compilable example before they proceed through the guide.

See this discussion for context. I will also apply similar changes to my in-progress API guides.

@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch 2 times, most recently from 7268705 to 151fa63 Compare March 23, 2023 21:35
Copy link
Copy Markdown
Contributor

@alexfornuto alexfornuto left a comment

Choose a reason for hiding this comment

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

I'm not a go programmer so I don't know if this makes sense, but if the sections that we break out and describe individually in these guides could be separate files imported into a main file, we could use partials inside the code blocks and ensure that the docs remain up-to-date if/when the examples change.

@ptgott
Copy link
Copy Markdown
Contributor Author

ptgott commented Mar 29, 2023

@alexfornuto I think that's a really good idea, though it will result in a slightly strange-looking program, since we'll have a collection of very small .go files. What do you think, @zmb3 ?

Edit: another aspect of this is that we would need to include package main at the top of each partial, which will also look strange within the guide itself.

@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch 2 times, most recently from b129612 to 013198c Compare April 5, 2023 21:43
@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch 3 times, most recently from ec2487c to 00155b8 Compare April 13, 2023 19:12
@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch from 00155b8 to e69d130 Compare April 25, 2023 21:24
@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch from e69d130 to 5b09226 Compare May 4, 2023 20:24
Copy link
Copy Markdown
Contributor

@Joerger Joerger left a comment

Choose a reason for hiding this comment

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

This is an awesome guide and sample project!

I'm not a go programmer so I don't know if this makes sense, but if the sections that we break out and describe individually in these guides could be separate files imported into a main file, we could use partials inside the code blocks and ensure that the docs remain up-to-date if/when the examples change.

+1 to this idea. I wouldn't split it into 10 files or anything, but I think you could reasonbly chunk it like this or similar:

  • func main
  • run and handleEvent
  • updateSpreadsheet
  • updateRow and createRow
  • makeRowData

... another aspect of this is that we would need to include package main at the top of each partial, which will also look strange within the guide itself.

IMO overall it's a worthwhile tradeoff, and as a Go programmer I wouldn't be confused by it.

Comment thread examples/access-plugin-minimal/go.mod
Comment thread examples/access-plugin-minimal/main.go Outdated
panic(err)
}

gs := googleSheetsPlugin{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: From the perspective of using this example as a base for one's own custom plugin, it would be cool to make the code a bit more modular. For example, rather than having a googleSheetsPlugin, we could have something like:

type accessRequestPlugin struct {
	teleportClient *client.Client
        eventHandler interface {
                HandleEvent(ctx context.Context, event types.Event) error
        }
}

type googleSheetsClient struct {
        sheetsClient   *sheets.SpreadsheetsService
}

func (*googleSheetsClient) HandleEvent(ctx context.Context, event types.Event) error {
       ...
}

plugin := accessRequestPlugin{
       teleportClient: ...,
       eventHandler: googleSheetsClient{
              sheetsClient: ...,
       }
}

Then the user could just swap out eventHandler with their own implementation very intuitively. This would also make splitting the code into separate files more natural.

I think this PR is a big improvement regardless, so this is more of a follow up suggestion than a blocker.

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.

This sounds great, especially if we want to make it the recommended way for a user to build/test their own Access Request plugin. If this is the way we want to go, I can look into making this change.

When writing this guide, though, I had imagined the example as something that would demonstrate some key libraries in a runnable way, but not the basis for an actual production plugin. Is there anything else we'd need to add to the accessRequestPlugin type to make it more generally usable?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nothing off the top of my head, just having a teleport Client and some customizable event handler seems sufficient as a base.

@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch from 9d62f53 to c87caa8 Compare May 18, 2023 16:04
@ptgott ptgott requested a review from Joerger May 18, 2023 16:30
@ptgott
Copy link
Copy Markdown
Contributor Author

ptgott commented Jun 15, 2023

Going to put this one in draft and plan to rework the example application so it doesn't rely on watcherjob. Instead, we can use teleport/api/types.Events.

@ptgott ptgott marked this pull request as draft June 15, 2023 21:53
@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch from 9765142 to d557902 Compare June 20, 2023 15:52
@ptgott ptgott marked this pull request as ready for review June 20, 2023 15:53
@ptgott
Copy link
Copy Markdown
Contributor Author

ptgott commented Jun 20, 2023

@Joerger I've finished incorporating your feedback by declaring the types you suggested! I've also added changes to address #27696 by eschewing the watcherjob package for types from the api package.

Copy link
Copy Markdown
Contributor

@alexfornuto alexfornuto left a comment

Choose a reason for hiding this comment

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

Copy changes look good, with one nit.

Comment thread docs/pages/api/access-plugin.mdx Outdated
ptgott and others added 9 commits July 6, 2023 13:36
This way, we can reuse the actual program in the Access Request plugin
API guide and avoid unintended discrepancies and drift.
Need to test this out, but it compiles
- Types that are no longer reachable via public interfaces
- The description of the demo implementation that used the old
  `watcherjob` package
Respond to Joerger feedback
Co-authored-by: Brian Joerger <bjoerger@goteleport.com>
- Split up "types.go". Reserve a single file for configuration values so
  these are visible in a single place within the guide.
- Return an error on an unsuccessful HTTP request when creating or
  updating a row
- Simplify requestStates lookup
- Clearly mark values that a user must change
- Update the text of the guide to match changes to the program
@ptgott ptgott force-pushed the paul.gottschling/2023-03-17-access-plugin branch from 1d3667d to 63f10ac Compare July 6, 2023 17:37
@ptgott
Copy link
Copy Markdown
Contributor Author

ptgott commented Jul 11, 2023

@zmb3 Friendly ping to see if I resolved your feedback

Comment thread examples/access-plugin-minimal/createrow.go Outdated
Comment thread examples/access-plugin-minimal/watcherjob.go Outdated
Comment thread examples/access-plugin-minimal/watcherjob.go Outdated
@ptgott ptgott enabled auto-merge July 14, 2023 18:36
@ptgott ptgott added this pull request to the merge queue Jul 14, 2023
Merged via the queue into master with commit 5cf2fa5 Jul 14, 2023
@ptgott ptgott deleted the paul.gottschling/2023-03-17-access-plugin branch July 14, 2023 19:12
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ptgott See the table below for backport results.

Branch Result
branch/v11 Create PR
branch/v12 Create PR
branch/v13 Create PR

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.

"How to Build an Access Request Plugin" guide doesn't work

4 participants