Skip to content
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

fix: create app instead of project on pspace init #48

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ps-kwang
Copy link
Contributor

I'm actually not clear this is correct because it requires the image and resources specified?

commands/init/mod.ts Outdated Show resolved Hide resolved
…nstead-of-v1projects' of https://github.com/Paperspace/cli into kwang/pla-2431-pspace-init-should-use-v1apps-endpoint-instead-of-v1projects
@@ -127,7 +135,7 @@ export const init = command("init", {
});

if (!flags.json) {
yield `✨ Created app "${app.name}"`;
yield `✨ Created app "${app.config.name}"`;
yield "";
yield fmt.colors.bold("Console URL");
yield new URL(
Copy link
Contributor

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"),
      ) + "";

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Screen Shot 2023-07-05 at 5 35 10 PM

This is what it looks like. You could try using it in the current version of the CLI where the benefits are clear.

asserts(res.ok, res);
app = res.data;
} else {
logger.info(
`Project already exists, skipping creation. (link: ${link.id})`,
);
const res = await projects.get({ id: link.id });
const res = await apps.get({ id: link.id });
Copy link
Contributor

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.

Copy link
Contributor

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

@ps-kwang ps-kwang marked this pull request as draft April 4, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants