-
Notifications
You must be signed in to change notification settings - Fork 365
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
feat: automatically link site post creation #3901
Conversation
7e7a21d
to
9b2b573
Compare
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.
Sorry for the late review but we where discussing this issue internally. But I think this solution should work pretty fine!
I tested it out and it is a way better DX.
Thanks for your effort and adding the tests 😍 I appreciate this a lot!
@erezrokah would it be a breaking change? As we change the behaviour of the existing link command or can it be seen as feature. Not sure if someone relies on the behaviour of NOT linking the site
} | ||
|
||
if (options.id) { | ||
} else if (options.id) { |
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 you please elaborate why you have changed this behaviour?
the exit should be the same behaviour.
IMHO early exits are easier to read then else if statements.
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.
Sure! I removed exit
because the sites-create
tests were failing since it was calling exit
even if everything was okay and made the test fail. By removing the exit
function I needed to make sure the other paths of the link
command weren't being executed, so I added the else if
to the conditions 😅
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.
In general I would try to not change the code because of tests.
As the exit function could be mocked in the tests and checked if it would be called.
But this is fine in this case
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #3827
When running the command
netlify sites:create
the CLI returns a success message with Admin URL, URL and Site ID. If we try to runnetlify build
we get an error "Could not find site ID. Please run netlify link".This change runs
link
after creating a site, as well as adding a flag--disable-linking
tosites:create
if we want to avoid linking the current directory.For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)