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

ADD a CLI for bootstrapping #1216

Merged
merged 28 commits into from
Aug 23, 2017
Merged

ADD a CLI for bootstrapping #1216

merged 28 commits into from
Aug 23, 2017

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jun 8, 2017

Issue: -bootstrapping process is becoming too much manual work-

What I did

I wrote a CLI for bootstrapping

IMAGE ALT TEXT HERE

How to test

run the following commands:

npm run bootstrap # will give a prompt
npm run bootstrap -- --all # will skip the prompt and bootstrap everything

TODO

  • Use npm run bootstrap -- --all on CI

@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Jun 8, 2017
@ndelangen ndelangen added this to the v3.0.2 milestone Jun 8, 2017
@ndelangen ndelangen self-assigned this Jun 8, 2017
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

See my suggestion above

});
}

if (program.docs) {
Copy link
Member

Choose a reason for hiding this comment

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

What about something like this to replace all of these if statements:

Object.keys(todo).forEach(key => {
  todo[key] = program[key] || program.all
})

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do this, thanks!

Copy link
Member

@theinterned theinterned left a comment

Choose a reason for hiding this comment

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

Looks good!

As per my comment, I wonder if the default behaviour should be to bootstrap all, with a flag to --pick or --interactive or --prompt which options to bootstrap.

But I am not very strong on that suggestion. It just seems like it may be more familiar to users.


program
.version('1.0.0')
.option('--all', 'Run all without asking')
Copy link
Member

Choose a reason for hiding this comment

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

Should --all be the default and instead a --interactive or --pick flag would launch the interactive picker?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm assuming developers want to type LESS ?

I think interactive mode is a sane default for a developer tool ?

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree ... just raised the consideration of keeping the normal bootstrap behaviour default with the interactive bootstrapping be a power mode. But this is more discoverable!

@shilman shilman modified the milestones: v3.1.2, v3.1.3, v3.1.4 Jun 9, 2017
@ndelangen
Copy link
Member Author

I need to rebase this, good opportunity to review npm install correctness /cc @tmeasday

@tmeasday
Copy link
Member

tmeasday commented Jul 5, 2017

"npm install correctness" -> Not quite sure what you mean by this @ndelangen? Do you mean in general or something that this PR is doing? (it doesn't seem to be touching our npm setup too much, AFAICT).

@ndelangen
Copy link
Member Author

npm 5.0.4 was released, so maybe we can get rid of npmc, is what I meant. See merge conflicts.

@tmeasday
Copy link
Member

tmeasday commented Jul 5, 2017

Makes sense to me, although I am not entirely clear on why we needed npmc to begin with.

@shilman
Copy link
Member

shilman commented Jul 22, 2017

@ndelangen can we close this now?

@ndelangen
Copy link
Member Author

No I will actually finish and add this soonish.

@ndelangen
Copy link
Member Author

Script is now also capable of reset task. It will do a git clean and reinstall root dependencies.

"npm run lint:md -- -o",
"git add"
]
"linters": {
Copy link
Member Author

Choose a reason for hiding this comment

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

I had trouble with lint-staged trying to git add multiple things in parallel, which fails because git's lockfile would exist.

I disabled concurrency for this reason. Depending on the amount of files in your stage, this will make the process a second or 2 slower, but git add is reliable.

Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen seems like this is a separate PR?

# Conflicts:
#	.circleci/config.yml
@Hypnosphi Hypnosphi self-requested a review August 23, 2017 11:01
@ndelangen
Copy link
Member Author

@shilman @storybooks/team can you approve ?

@ndelangen ndelangen closed this Aug 23, 2017
@ndelangen ndelangen reopened this Aug 23, 2017
"npm run lint:md -- -o",
"git add"
]
"linters": {
Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen seems like this is a separate PR?

option: '--reset',
command: () => {
log.info(prefix, 'git clean');
spawn('git clean -fdx');
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about just running lerna clean instead of git clean -fdx which is much more destructive?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be completely destructive to anything not managed by git. Anything added not known by or ignored by git should be removed. The desired end-result is as if you just cloned the repo.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, git clean -fdx breaks my IDE setup each time

Copy link
Member Author

@ndelangen ndelangen Aug 23, 2017

Choose a reason for hiding this comment

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

@Hypnosphi Can you give me a list / regexp that you IDE adds that should NOT be cleaned away?
https://git-scm.com/docs/git-clean#git-clean--eltpatterngt

});
process.stdout.write('\x07');
try {
spawn('say "Bootstrapping sequence complete"');
Copy link
Member

Choose a reason for hiding this comment

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

AWESOME 💯

Copy link
Member Author

Choose a reason for hiding this comment

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

Lol, I'm actually concerned this will be annoying to linux and windows users, will have to ask @usulpro maybe?

Copy link
Member

Choose a reason for hiding this comment

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

won't it just fail silently on those platforms?

log.info(prefix, 'git clean');
spawn('git clean -fdx');
log.info(prefix, 'yarn install');
spawn('yarn install --no-lockfile');
Copy link
Member

Choose a reason for hiding this comment

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

why no lockfile? i think yarn's lockfiles work pretty well cc @Hypnosphi

Copy link
Member Author

Choose a reason for hiding this comment

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

We're currently ignoring them, so not generating them in the first place is better.

Let's discuss enabling yarn lockfiles outside this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Let's do it: #1703

option: '--reset',
command: () => {
log.info(prefix, 'git clean');
spawn('git clean -fdx --exclude=".vscode" --exclude=".idea"');
Copy link
Member

Choose a reason for hiding this comment

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

Really have to object here. This is a destructive operation and there is no warning to the user.

Propose one or more of:

  • warning in name e.g. "CAUTION removes all files not associated with git!"
  • confirmation step: "This removes all files not associated with git. Are you sure?"
  • another bootstrap command (clean = lerna clean, reset = git clean -fdx with warnings)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look at adding a "Are you sure you want to do what you told me to do" confirmation screen.

Copy link
Member Author

@ndelangen ndelangen Aug 23, 2017

Choose a reason for hiding this comment

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

done!
screen shot 2017-08-23 at 21 07 06

Copy link
Member

Choose a reason for hiding this comment

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

It's not really clear, what are the files that will get deleted. Can those be my kitten gifs from personal directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, those will get deleted 🐈 🚫 /jk

Got a suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

State exactly what will be removed: all the files not present in git, except for .idea and .vscode. This is more verbose, but also more fair

Copy link
Member Author

Choose a reason for hiding this comment

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

screen shot 2017-08-23 at 23 04 35

Copy link
Member Author

Choose a reason for hiding this comment

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

screen shot 2017-08-23 at 23 17 01

@shilman shilman merged commit 599db93 into master Aug 23, 2017
@shilman
Copy link
Member

shilman commented Aug 23, 2017

@ndelangen great job on this! 🎯 💯 🎖

@shilman shilman deleted the improve-bootstrap-command branch August 23, 2017 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants