-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create Spacelift client #5
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.
I like the idea to reuse the client from spacectl 👍🏻
I just dropped a few comments but the overall approach looks great 👍🏻
a515328
to
55ac561
Compare
@michalg9 just to mention, the Prometheus exporter has a Spacelift GraphQL client in it that's basically a stripped-down version of the one from spacectl that only support API keys: https://github.com/spacelift-io/prometheus-exporter. Feel free to ignore it since you've already done this, but just mentioning it because it might be worth taking a look at. It has some fixes for things like refetching the token when it expires and retrying the request. |
RE the token refresh issue - here's what I'm talking about: spacelift-io/prometheus-exporter@380423d The problem is that spacectl isn't designed for long-running operations, so if you cache the session it stops working after a while. |
That's actually a very good concern @adamconnelly. We talked about that with @michalg9 and we thought that the spacectl client was handling the token refresh for us. But if not, we should probably use the prom version one 👍🏻 |
After discussing with @adamconnelly he suggested that even though the refresh token logic is in spacectl client, that.. I'll just quote the Man 😆 :
I'll swap the spacectl client with prometheus client, and then, after kubecon we can work on extracting that to a separate repo. |
Let's do that, good catch @adamconnelly
100% I created an entry for that in our sheet here to be sure we'll not forget |
Okay, I've refactored a bunch of things and added secret fetching, so that it should unblock @eliecharra and me with continuing with CRDs. I will handle the edge case of removing session in UI or due to login policy change in a separate PR to unblock our work (CC @adamconnelly ) |
var query struct { | ||
Stack struct { | ||
ID string `graphql:"id"` | ||
Administrative bool `graphql:"administrative"` | ||
} `graphql:"stack(id: $id)"` | ||
} | ||
|
||
variables := map[string]interface{}{ | ||
"id": graphql.ID("end-to-end-autoconfirm"), | ||
} | ||
|
||
err = spaceliftClient.Query(context.Background(), &query, variables) | ||
if err != nil { | ||
return ctrl.Result{}, err | ||
} |
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.
I believe we can enclose queries we will use in some nice functions in client package, for now that's just an usage example, wanted to make sure it works e2e.
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 I was gonna drop a comment about that, we can still refactor later on but I would like to have a internal/spacelift/repository/stack.go
and internal/spacelift/repository/run.go
to abstract that from the reconcilier.
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.
I know you guys are gonna tweak this, but +1 from me to unblock you for now. Overall approach is fine.
var query struct { | ||
Stack struct { | ||
ID string `graphql:"id"` | ||
Administrative bool `graphql:"administrative"` | ||
} `graphql:"stack(id: $id)"` | ||
} | ||
|
||
variables := map[string]interface{}{ | ||
"id": graphql.ID("end-to-end-autoconfirm"), | ||
} | ||
|
||
err = spaceliftClient.Query(context.Background(), &query, variables) | ||
if err != nil { | ||
return ctrl.Result{}, err | ||
} |
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 I was gonna drop a comment about that, we can still refactor later on but I would like to have a internal/spacelift/repository/stack.go
and internal/spacelift/repository/run.go
to abstract that from the reconcilier.
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.
Amazing @michalg9, thanks a lot for that 🙏🏻
Let's merge it asap so we can unblock run and stack reconcilier work, and refactor it a bit later.
I actually got an idea to make the client a bit easier to use, I'll push a PR soon
The most simple solution that is basically re-using code we have in spacectl.
The main idea for dropping it is because we'd need to re-implement Spacelift token fetching. There are also other challenges (e.g. you need to have whole graphql schema on a file locally to generate safe-type structs) and all this seems like a big overkill for a simple POC we need to deliver in 2 weeks.
Happy to take the idea of writing an SDK later, as indeed it would be useful if all our tooling (e.g. tf provider, spacectl, this) used something common.