COMPASS-665 Decouple DDL into new package #752

pzrq · 2017-01-20T05:51:49Z

Just looking for a quick pair of 👀 over this and hopefully an LGTM / merge.

This is an enabler for COMPASS-558 (see #745).

The only user-facing changes should be below - making the Drop modals more aware of the database to be dropped.

Drop Database Before

Drop Database After

Drop Collection Before

Drop Collection After

pzrq · 2017-01-21T00:23:17Z

@rueckstiess Just following up our chat before the Share & Learn.

I agree that ddl is probably not an appropriate final name for this package in light of sortDatabases being the lone non-ddl action suggesting we do want a different superset name, however my thinking remains that we want to get COMPASS-558 done this sprint and so:

naming things 😄
done is better than perfect
this change set decouples ddl or whatever we choose to name it from the server-stats package where it definitely does not belong
ddl is almost unique and thus relatively easy to search for and rename again

Could I suggest creating a 1-3 story point ticket for our next sprint (which I'd be happy to pick up), ideally one that clarifies:

the instance (e.g. App.InstanceActions => instance package perhaps?) vs
databases vs database (ddl vs database internal packages, perhaps renaming the ddl internal-package to instance as suggested) vs
collections vs collection(e.g. collections-store vs collection-store)
any other things that show up in grooming such a ticket

If that's not acceptable please let me know the way you'd like us to proceed forward, thanks 👍

pzrq · 2017-01-21T00:50:05Z

Rebased on master to resolve merge conflicts with #749

rueckstiess · 2017-01-23T00:05:06Z

Just pick one of:

database-ddl
databases-table
databases

rueckstiess · 2017-01-23T00:15:35Z

src/internal-packages/database/styles/drop-collection-dialog.less

 }
+.drop-confirm-namespace {
+  font-family: Menlo, Monaco, Consolas, "Courier New", monospace;
+  font-size: 90%;


not sure about relative font sizes. I don't think we use that anywhere else. Let's use an absolute one, in px or em.

👍 changed to 13px

rueckstiess · 2017-01-23T00:17:39Z

src/internal-packages/ddl/package.json

+{
+  "name": "ddl",
+  "productName": "Compass Data Definition Language",
+  "description": "Data Definition Language (DDL) operations for Compass",


Let's change the name to one of

database-ddl

databases-table

databases

and update the description as well. Otherwise it's really confusing as it doesn't actually contain any DDL except for databases.

👍 database-ddl works for me

rueckstiess · 2017-01-23T00:17:58Z

src/internal-packages/ddl/styles/drop-database-dialog.less

+
+.drop-confirm-namespace {
+  font-family: Menlo, Monaco, Consolas, "Courier New", monospace;
+  font-size: 90%;


see above. use absolute font size.

👍 changed to 13px

pzrq · 2017-01-23T03:25:59Z

@rueckstiess 👍 Thanks for the feedback, ready for the next round of review.

rueckstiess · 2017-01-24T00:34:15Z

LGTM but needs rebase.

… arg Which means it can be reused in other contexts like the sidebar (incoming).

Plurals just lead to confusion.

Tooltips finally work (when integrated with COMPASS 558).

Tooltips almost finally work (when integrated with COMPASS 558). Incoming soon: The drop collection assumes you are managing the current database, shouldn’t be too hard to refactor :)

For consistency with drop collection.

Plurals are confusing and explicit is preferred over implicit, as we already have a database internal-package and the modals are no longer hard-wired to the database-table as they will shortly be able to be triggered from the sidebar.

pzrq · 2017-01-24T00:49:56Z

Thanks @rueckstiess, hopefully you're starting to see why I think getting review done fast is a good thing, less rebasing followed by waiting on Travis and retrying if functional tests fail 👍

#retry COMPASS-635 https://travis-ci.com/10gen/compass/jobs/62828516

pzrq · 2017-01-24T02:29:35Z

Woohoo Travis finally passed!

pzrq force-pushed the COMPASS-665-decouple-ddl branch 2 times, most recently from f9fa46c to 6e6494d Compare January 20, 2017 23:53

pzrq requested a review from rueckstiess January 21, 2017 00:23

pzrq added the ready for review label Jan 21, 2017

pzrq force-pushed the COMPASS-665-decouple-ddl branch from 6e6494d to 5f4c925 Compare January 21, 2017 00:37

pzrq mentioned this pull request Jan 21, 2017

COMPASS-558 Add DDL buttons #745

Merged

18 tasks

rueckstiess suggested changes Jan 23, 2017

View reviewed changes

pzrq force-pushed the COMPASS-665-decouple-ddl branch 2 times, most recently from 846d79c to 634bf0d Compare January 23, 2017 02:17

pzrq added work in progress and removed ready for review labels Jan 23, 2017

pzrq force-pushed the COMPASS-665-decouple-ddl branch from 634bf0d to 048586f Compare January 23, 2017 02:59

pzrq added ready for review and removed work in progress labels Jan 23, 2017

rueckstiess approved these changes Jan 24, 2017

View reviewed changes

pzrq added 10 commits January 24, 2017 11:47

Refactor: CreateCollectionDialog.onOpenDialog requires a databaseName…

bf24099

… arg Which means it can be reused in other contexts like the sidebar (incoming).

Rename to action/component in home, dropping plurals

0a8a6c2

Plurals just lead to confusion.

Decouple DDL internal-package from server-stats

b3b70b3

Promote Create/DropDatabaseDialog to Home

e76d9fe

Tooltips finally work (when integrated with COMPASS 558).

Promote Create/DropCollectionDialog to Home

651c9c1

Tooltips almost finally work (when integrated with COMPASS 558). Incoming soon: The drop collection assumes you are managing the current database, shouldn’t be too hard to refactor :)

Generalise drop collection modal to require the databaseName

7dc6545

Fix bug: Drop collection not working

fc96c9b

Update tests for decoupled DDL package

ffbea63

Restyle drop database modal

8b447d0

For consistency with drop collection.

Use fixed font size recommended in code review

48bf191

Rename ddl to database-ddl

4305016

Plurals are confusing and explicit is preferred over implicit, as we already have a database internal-package and the modals are no longer hard-wired to the database-table as they will shortly be able to be triggered from the sidebar.

pzrq force-pushed the COMPASS-665-decouple-ddl branch from 4fea6c1 to 67a6783 Compare January 24, 2017 00:48

More DDL -> DatabaseDDL renaming

3678088

#retry COMPASS-635 https://travis-ci.com/10gen/compass/jobs/62828516

pzrq force-pushed the COMPASS-665-decouple-ddl branch from 67a6783 to 3678088 Compare January 24, 2017 01:05

pzrq merged commit 1ec3049 into master Jan 24, 2017

pzrq deleted the COMPASS-665-decouple-ddl branch January 24, 2017 02:29

COMPASS-665 Decouple DDL into new package #752

COMPASS-665 Decouple DDL into new package #752

Uh oh!

Conversation

pzrq commented Jan 20, 2017

Uh oh!

pzrq commented Jan 21, 2017

Uh oh!

pzrq commented Jan 21, 2017

Uh oh!

rueckstiess commented Jan 23, 2017

Uh oh!

rueckstiess Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

pzrq Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

rueckstiess Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

pzrq Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

rueckstiess Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

pzrq Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

pzrq commented Jan 23, 2017

Uh oh!

rueckstiess commented Jan 24, 2017

Uh oh!

pzrq commented Jan 24, 2017

Uh oh!

pzrq commented Jan 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants