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 command line tool for adding multiple office users #2355

Merged
merged 12 commits into from Jul 12, 2019

Conversation

4 participants
@mkrump
Copy link
Contributor

commented Jul 5, 2019

Description

Right now, a manually generated migration is required to add new office users. When there are many new users to add writing these migrations by hand can be tedious and error prone. Eventually, the admin interface will replace this process, but in the meantime it'd be nice to have a script that could help generate these migrations.

Note:
I wasn't completely sure that there was a real need here (feel free reject if not), but last week we'd added 13 new office users and writing the SQL by hand involved a lot of copying and pasting from the spreadsheet we were sent. It seemed like a script would simplify this a bit.

Setup

Generate a sample list of fake test users and save in a csv file formatted like below.

| First Name | Middle Initials (Optional) | Last Name | Email                    | Phone          | Transport Office (UUID)               |
|------------|----------------------------|-----------|--------------------------|----------------|---------------------------------------|
| Robert     | T                          | Sanders   | robertsanders@mail.com   | (915) 269-1070 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| Jennifer   |                            | Jackson   | jenniferjackson@mail.com | (201) 271-0070 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| Robert     |                            | Diaz      | robertdiaz@mail.com      | (241) 740-2961 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| John       |                            | Cooper    | johncooper@mail.com      | (750) 789-5810 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| Robert     |                            | Thompson  | robertthompson@mail.com  | (346) 464-0904 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| Lisa       |                            | Morgan    | lisamorgan@mail.com      | (225) 656-6220 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| Richard    |                            | Ellis     | richardellis@mail.com    | (853) 992-5796 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| David      | J                          | Gonzalez  | davidgonzalez@mail.com   | (790) 907-0453 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| Richard    |                            | Collins   | richardcollins@mail.com  | (518) 772-8852 | 313db258-d067-41d1-bbc2-91023d62f9a3  |
| Barbara    |                            | Howard    | barbarahoward@mail.com   | (392) 216-7523 | 313db258-d067-41d1-bbc2-91023d62f9a3 |

make build_tools
./bin/make_office_users_migration -f test_office_users.csv -n test-migration
# two new files "tmp/20190711163046_test-migration.up.sql" and "migrations/20190711163046_test-migration.up.fizz" will be created.

make build_server
milmove gen office-user-migration -f ~/Desktop/office_users.csv -n test-migration

This will do 4 things:

  1. generate the secure migration to be uploaded at tmp/20190711202918_test-migration.up.sql
  2. generate an empty migration at local_migrations/20190711202918_test-migration.up.sql
  3. generate the file to apply the secure migration at migrations/20190711202918_test-migration.up.fizz
  4. add the new migration in 3. to the ./migrations_manifest.txt

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • Request review from a member of a different team.

References

@mkrump mkrump added the wip label Jul 5, 2019

Makefile Outdated
@@ -241,6 +241,9 @@ bin/load-office-data: .server_generate.stamp
bin/load-user-gen: .server_generate.stamp
go build -ldflags "$(LDFLAGS)" -o bin/load-user-gen ./cmd/load_user_gen

bin/make-office-users: .server_generate.stamp

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

There's already a make-office-user so to avoid confusion maybe pick make-office-user-migration?

This comment has been minimized.

Copy link
@mkrump

mkrump Jul 8, 2019

Author Contributor

👍

cli.InitVerboseFlags(flag)

// Add Office Users
cli.InitAddOfficeUsersFlags(flag)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Contributor

When the flags are likely to be only used in one binary its ok to keep them in this file instead of adding them to github.com/transcom/mymove/pkg/cli. It's a rough rule but it's helped keep things a bit cleaner.

This comment has been minimized.

Copy link
@mkrump

mkrump Jul 8, 2019

Author Contributor

makes sense. moved into main.

@mkrump mkrump force-pushed the mk-make-office-users branch from a607702 to 79cd009 Jul 8, 2019

@mkrump mkrump changed the title WIP - Add command line tool for adding multiple office users Add command line tool for adding multiple office users Jul 8, 2019

@mkrump mkrump requested review from jim, mikena-truss and donaldthai Jul 8, 2019

@mkrump mkrump added 🦇team and removed wip labels Jul 8, 2019

@mkrump mkrump marked this pull request as ready for review Jul 8, 2019

@mkrump mkrump requested a review from tinyels Jul 8, 2019

@tinyels

tinyels approved these changes Jul 8, 2019

Copy link
Contributor

left a comment

This is great! Perhaps we could create a csv template for product so the data just comes in the format we need?

@chrisgilmerproj
Copy link
Contributor

left a comment

Nice job!

Could you add to the ticket an example of a migration and its corresponding CSV? Also, I'd love to see you add a "How To" document on how to use this. I imagine the next one of these we'll see is a script to generate TSP users and I'd love for them to have some usage info to work from.

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

This is great! Perhaps we could create a csv template for product so the data just comes in the format we need?

@tinyels makes sense. how should i make the template available to product?

const (
//OfficeUsersFilenameFlag filename containing the details for new office users
OfficeUsersFilenameFlag string = "office-users-filename"
OfficeUsersMigrationFile = "add_office_users_migration.sql"

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 8, 2019

Contributor

How about creating a flag for this so people can choose to use a different name if they want? additionally, there's a pattern for how these look in the migrations/ folder we could use.

This comment has been minimized.

Copy link
@mkrump

mkrump Jul 8, 2019

Author Contributor

sure. The one thing that I was thinking here is that this wouldn't replace soda for generating the migration file, but instead would just allow us to copy paste this file into the actual migration. Maybe I should name it tmp_add_officer_users.sql and then ignore in our .gitignore. also open to changing, but might need a little more detail over slack.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

I'd actually love to deprecate the use of soda, so why not skip that part and just write a sql file directly into the migrations directory with the right name and everything? Making this as easy as possible to use would probably increase adoption. Plus it eliminates any mistakes from copy/pasta.

This comment has been minimized.

Copy link
@mkrump

mkrump Jul 9, 2019

Author Contributor

ok. So are you thinking that we'd have a separate script similar to generate-secure-migration here https://github.com/transcom/mymove/blob/master/docs/how-to/migrate-the-database.md#secure-migrations? And i guess in this case we'd just copy the output of this command into the prod migration that goes into tmp?

This comment has been minimized.

Copy link
@mkrump

mkrump Jul 11, 2019

Author Contributor

@chrisgilmerproj I need to clean up / test a bit more, but what about the updated version? It will generate two new migration files like generate-secure-migration.

The secure migration will show up in tmp with the raw sql, while the fizz apply secure migration will be added to the local migrations. I borrowed some of what patrick was doing in his gen migration script, so thanks for linking to that.

This comment has been minimized.

Copy link
@tinyels

tinyels Jul 11, 2019

Contributor

I'd like to have this available for the pilot next week and am not sure it needs to be perfect for that. It's also work that will be deprecated/removed as soon as users can be created in the admin console so let's not over engineer it.

This comment has been minimized.

Copy link
@mkrump

mkrump Jul 11, 2019

Author Contributor

ok. i added model validation and accounted for nullable middle initial, but i can stop fiddling with this.

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Nice job!

Could you add to the ticket an example of a migration and its corresponding CSV? Also, I'd love to see you add a "How To" document on how to use this. I imagine the next one of these we'll see is a script to generate TSP users and I'd love for them to have some usage info to work from.

@chrisgilmerproj Added an example to the How-To e469ca3?short_path=78d1cbf#diff-78d1cbfa8fd79e7f2607b4f772746d24. Will update the pivotal ticket as well to link to that section as well.

mkrump added some commits Jul 9, 2019

WIP
WIP: Updating scripts/generate-office-users-migration to work with new command
@tinyels

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

This is great! Perhaps we could create a csv template for product so the data just comes in the format we need?

@tinyels makes sense. how should i make the template available to product? I think you could create a stub in g-drive and let them know about it.

Makefile Outdated
@@ -241,6 +241,9 @@ bin/load-office-data: .server_generate.stamp
bin/load-user-gen: .server_generate.stamp
go build -ldflags "$(LDFLAGS)" -o bin/load-user-gen ./cmd/load_user_gen

bin/make-office-users-migration: .server_generate.stamp

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 11, 2019

Contributor
Suggested change
bin/make-office-users-migration: .server_generate.stamp
bin/make_office_users_migration: .server_generate.stamp
PR Feedback:
* Move into milmove gen as a subcommand
* Add migration to manifest
* Create an empty local_migration

@mkrump mkrump force-pushed the mk-make-office-users branch from ed7c3d5 to 86bedfb Jul 11, 2019

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@chrisgilmerproj most recent commit 86bedfb I think should address the comments from discussion that we had over slack.

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented on cmd/milmove/gen_office_user_migration.go in 86bedfb Jul 11, 2019

will fix spelling

Show resolved Hide resolved cmd/milmove/main.go Outdated
Show resolved Hide resolved cmd/milmove/main.go Outdated
@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented on cmd/milmove/gen_office_user_migration.go in 86bedfb Jul 11, 2019

not using this anymore, so going to remove

Show resolved Hide resolved cmd/milmove/main.go Outdated
Show resolved Hide resolved cmd/milmove/main.go Outdated
@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - Nice job!

@jim

jim approved these changes Jul 12, 2019

Copy link
Contributor

left a comment

I tested this locally and it worked as intended.

I did run into an issue with either the command line library or how we're using it, which is that when you try to run a command that doesn't exist, you get an error about a flag instead of a command:

jimb@wfh ~/c/mymove (mk-make-office-users) [2]> milmove gen officer-user-migration -f ~/Desktop/office_users.csv -n add-more-office-users
Error: unknown shorthand flag: 'f' in -f
Usage:
  milmove gen [command]

Available Commands:
  migration             Generate migrations and other objects
  office-user-migration Generate migrations required for adding office users

Flags:
  -h, --help   help for gen

Use "milmove gen [command] --help" for more information about a command.

panic: unknown shorthand flag: 'f' in -f

goroutine 1 [running]:
main.main()
	/Users/jimb/code/mymove/cmd/milmove/main.go:99 +0x90b

I don't think this is blocking, but it would be nice to address.

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

I tested this locally and it worked as intended.

I did run into an issue with either the command line library or how we're using it, which is that when you try to run a command that doesn't exist, you get an error about a flag instead of a command:

This seems to be an issue with all subcommands + Cobra (see this GH issue Consistency in handling unknown commands/Expose findSuggestions(). I wasn't following the quick fix the user was suggesting, so I might defer on this. I'm subscribed to that issue now, so hopefully someone will reply with how this should be handled in Cobra and if not can revisit if that's ok.

@mkrump mkrump merged commit 2b6a5cd into master Jul 12, 2019

19 checks passed

ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing d8584e8...402287b
Details
codecov/project/go 59.52% remains the same compared to d8584e8
Details

@mkrump mkrump deleted the mk-make-office-users branch Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.