-
Notifications
You must be signed in to change notification settings - Fork 5
chore(Coder plugin): import preview version of Coder SDK into plugin #130
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
| 'custom-roles' | ||
| 'example' | ||
| 'multi-organization'; | ||
export const experiments: Experiment[] = [ |
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.
Think I'm going to try tightening up the ESLint rules in core, and try to ping Steven on updating the type generation logic
Backstage's version did catch something a little confusing, which is that in Core, we have an Experiments
type and an Experiments
runtime array. But you're stuck doing aliased imports if you ever need both in the same file. Going to see if there's a way to rename the array to be lowercase. The same thing also affects Entitlements
@code-asher Turning this back into a draft really quickly, because I'm realizing that I want to try hiding some of the methods from the core Coder stuff that wouldn't ever be relevant to a Backstage user (things like JFrog) |
type PropertyToHide = | ||
| 'getJFrogXRayScan' | ||
| 'getCsrfToken' | ||
| 'setSessionToken' | ||
| 'setHost' | ||
| 'getAvailableExperiments' | ||
| 'login' | ||
| 'logout'; |
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.
Wasn't sure if you thought there were more/less properties that it would make sense to hide?
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.
Looks good to me although to be honest I did not spend a whole lot of time looking through everything we are exporting.
I wonder though if we want to move these to an InternalApiMethods
or a helpers file or something so we will not have to filter them out here.
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 went through it a bit more after all haha
convertToOAUTH
probably would make sense to hide as well.
I am not sure about waitForBuild
. It is a helper function, not a straight API call. Maybe we exclude helpers for now but can think about exposing them differently.
Similarly we have a bunch of convenience wrappers around the postWorkspaceBuild
endpoint. At the very least I think we should expose them differently, if at all (the Go sdk does not have equivalents for example, as far as I know).
addMember
and removeMember
are also wrappers/helpers that do not exist on the Go sdk.
changeWorkspaceVersion
, updateWorkspace
, these are somewhat bespoke for our dashboard, not necessarily that they would not be useful for users, but not really API methods either.
getWorkspaceParameters
seems to just combine two other calls.
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 would normally agree that it'd be better to hide these things, but my hope with the update process was that you could literally copy-and-paste the files from core with zero changes, and have things just work
I'm going to exclude the methods you highlighted, but keep everything else the same for now. If we're going to need better separation, I feel like those changes should be made in core
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.
Oh yeah 100% agree those changes should be made in core, not here.
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.
Can be ignored; copied over from core with zero changes
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.
Exciting!
Scaffolding needed for #114
Changes made
Notes
api
sectionapi.ts
andtypesGenerated.ts