Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Pin package versions to match Teleport v12#773

Merged
justinas merged 5 commits intomasterfrom
justinas/v12-mod-fixes
Mar 6, 2023
Merged

Pin package versions to match Teleport v12#773
justinas merged 5 commits intomasterfrom
justinas/v12-mod-fixes

Conversation

@justinas
Copy link
Copy Markdown
Contributor

We are now importing teleport-plugins in the teleport repo. However, teleport's v12 branch is holding back packages related to Firestore due to incompatibilities (this has since been solved in master by gravitational/teleport#21190).

To avoid bumping these packages in teleport, this PR pins the packages in teleport-plugins as well.

Comment thread go.mod
golang.org/x/net v0.7.0
golang.org/x/sync v0.1.0
google.golang.org/grpc v1.53.0
google.golang.org/grpc v1.52.3
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.

grpc downgraded because it was pulling a newer genproto version. Rest of the changes (other than the "pinned" block) were made by go mod tidy.

@justinas justinas marked this pull request as ready for review February 27, 2023 15:36
Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis left a comment

Choose a reason for hiding this comment

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

Maybe we should re-use the teleport repo model here
master is the next release and have a branch/v12 🤔

@marcoandredinis
Copy link
Copy Markdown
Contributor

We are now importing teleport-plugins in the teleport repo.

I would rather have this the other way around 🤔 I guess we are a bit late now
Teleport Plugins is already importing Teleport source code
And now Teleport is also importing Teleport Plugins which creates a cyclic dependency (might not be breaking anything technically but is still, IMO, a bad approach)

@justinas
Copy link
Copy Markdown
Contributor Author

We are now importing teleport-plugins in the teleport repo.

I would rather have this the other way around 🤔 I guess we are a bit late now Teleport Plugins is already importing Teleport source code And now Teleport is also importing Teleport Plugins which creates a cyclic dependency (might not be breaking anything technically but is still, IMO, a bad approach)

One good thing is, technically the dependency is not circular, as teleport-plugins imports only teleport/api, which is a different Go module 😅

Importing teleport-plugins is something we need to do in teleport (more precisely, teleport.e) to implement Hosted plugins. I can't think of an architecture for this in which the imports would flow the other way (teleport-plugins -> teleport). 🤔

@marcoandredinis
Copy link
Copy Markdown
Contributor

Yeah, you've got a point there: teleport-plugins only imports the teleport/api 👍

I guess we would need to move teleport-plugins/access/common/ into teleport/api and then we could import it from plugins and from teleport itself.

I'm not entirely sure of the teleport-plugins goals from an architectural standpoint.
However, I had the impression that it was meant to be a set of plugins that we would provide as our offering but also as inspiration for the community to create their own.
Something entirely decoupled from the teleport repo (aside from the api/ module).

Well, that's not a huge deal. Just a change of my personal perception of what depends on what.
Just something that seemed odd at first, but no real problem here.
We were also thinking of moving (some ?) plugins to teleport repo, so this discussion is already deprecated 😅

@r0mant
Copy link
Copy Markdown
Contributor

r0mant commented Feb 28, 2023

We are now importing teleport-plugins in the teleport repo.

@justinas @marcoandredinis This might be a good thing, as we're working on OpsGenie plugin that is the first plugin that's going to live in Teleport repo and be a part of teleport binary. Then we'll migrate other plugins over as well. cc @EdwardDowling

@justinas
Copy link
Copy Markdown
Contributor Author

justinas commented Mar 3, 2023

@r0mant could I get your approval on this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants