-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
...-endpoint-instead-of-v1projects
...nstead-of-v1projects' of https://github.com/Paperspace/cli into kwang/pla-2431-pspace-init-should-use-v1apps-endpoint-instead-of-v1projects
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 whoops, this URL is wrong now. Should be
yield new URL( `/${team}/apps/${app.id}`, env.get("PAPERSPACE_CONSOLE_URL"), ) + "";
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.
Also I think the message in general is off because calling this command will trigger a deploy. I wonder if that's actually desirable. I mean the alternative is to not actually call apps.create({ config }) here I guess, but then there's no project linking etc.
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 was also a little surprised by that. I wouldn't expect that initializing a cli would deploy something. But if we remove that then is the command really necessary?
It seems like maybe init is not the right name, it sounds more like bootstrap or something?
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.
Ya we need to remove the upsert from the init api and just set up project linking. The command is definitely necessary and named correctly 😄 It's similar to docker init in that it gets you set up with all of the necessary files to start your project. It has the additional step of linking your local directory to a project ID which lets you drop --projectId flags from commands.
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.
This is what it looks like. You could try using it in the current version of the CLI where the benefits are clear.
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.
IDK if this line or L133 is actually right anymore either because I think it'd have to be id: app.projectId. Linking is going to be super wonky now because when you get an app, you're not getting it by project ID (currently) you're getting it by the base deployment UUID.
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.
might need to make an API change to make this work
I'm actually not clear this is correct because it requires the image and resources specified?