Remove pop from Proxy & RDBMS storage getter #511

manugupt1 · 2018-08-17T23:43:42Z

This PR is huge. I did not really want it that way, however as I was making CI (Travis work), I was making changes and was in a complete rabbit hole. If required, I can always separate it out it in multiple ways and in the process.

It makes the proxy Pop free
It keeps olympus using pop as CDN is a part of olympus and it has an RDBMS driver with tests
It will update the CI to use a postgres database as this commit from @arschles removes mysql https://github.com/gomods/athens/pull/487/files
It updates the migration files to have postgres dump so that it can be streamlined again.
It changes default docker-compose image to use for postgres to use postgres and postgres which is the default and is used as most commonly for most systems that are not docker based, so it streamlines dev build using docker and otherwise when you look at database.yml.
It removes unneeded code from cdn driver
It changes the rdbms cdn driver to use test as the test unless GO_ENV is specified otherwise

Fixes #496
Fixes #494
Fixes #450
Fixes #487

- Not sourcing from cmd/proxy/.env, because that doesn't export any variables - Removing mysql support (I feel like 1 database is enough) - Pruning networks on teardown I'm happy to split these changes up into separate PRs - it's the end of the day for me so I wanted to get everything in 😄

codecov-io · 2018-08-18T18:53:39Z

Codecov Report

Merging #511 into master will increase coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master     #511    +/-   ##
========================================
+ Coverage    39.9%   40.91%    +1%     
========================================
  Files         112      104     -8     
  Lines        3205     3065   -140     
========================================
- Hits         1279     1254    -25     
+ Misses       1799     1688   -111     
+ Partials      127      123     -4

Impacted Files	Coverage Δ
cmd/olympus/actions/storage.go	`0% <ø> (ø)`	⬆️
cmd/olympus/actions/app.go	`69.15% <ø> (-1.38%)`	⬇️
cmd/proxy/actions/app.go	`59.43% <ø> (ø)`	⬆️
cmd/proxy/actions/storage.go	`16.66% <ø> (+1.85%)`	⬆️
cmd/olympus/models/models.go	`100% <100%> (+37.5%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8b64fd...97ea321. Read the comment docs.

rohancme · 2018-08-19T19:11:18Z

cmd/proxy/models/models.go

@@ -1,22 +1,5 @@
 package models


do we need these 2 files at all after this change?

I do not think so at all. I have kept it simply because I am coming from an RoR space where convention over configuration. Buffalo by default creates models.go and that is why I have kept it around.

If we want we can remove it!

michalpristas · 2018-08-20T10:34:46Z

cmd/olympus/actions/storage.go

@@ -34,12 +33,6 @@ func GetStorage() (storage.Backend, error) {
 			return nil, err
 		}
 		return mongo.NewStorage(mongoURI)
-	case "postgres", "sqlite", "cockroach", "mysql":


we are removing postgres support, is that right? we keep postgres image running and we have olympus database yaml saying dialect: "postgres"

We need to have Postgres image and the yml file on Olympus because we have a CDN RDBMS driver. This way, we can run tests for CDN but still remove RDBS storage support from pkg/

michalpristas · 2018-08-20T18:59:38Z

cmd/proxy/models/models.go

-// DB is a connection to your database to be used
-// throughout your application.
-var DB *pop.Connection
-
 func init() {


please remove init(), there's no point of having it

michalpristas · 2018-08-20T19:00:02Z

cmd/proxy/models/models_test.go

 func Test_ModelSuite(t *testing.T) {
-	as := &ModelSuite{suite.NewModel()}
-	suite.Run(t, as)
+


m: whitespace

arschles · 2018-08-20T19:04:14Z

@manugupt1 a have the nitpickey-ist of all nitpicks, sorry in advance!

Instead of writing:

Fixes the following

#496
#494
#450
#487

Can you write this:

Fixes #496
Fixes #494
Fixes #450
Fixes #487

This way, GitHub will properly close all 4 of those issues when this PR is merged.

marwan-at-work

Just one tiny change about the Makefile.

And the rest are questions

marwan-at-work · 2018-08-20T21:33:08Z

.travis.yml

@@ -20,8 +20,9 @@ before_script:
  - make setup-dev-env
  - wget "https://dl.minio.io/server/minio/release/linux-amd64/minio"
  - chmod +x minio && nohup ./minio server . &
-  - buffalo db create
-  - buffalo db migrate up
+  - buffalo db create -e $GO_ENV -d -c $POP_PATH/database.yml


why do we need this if we're removing pop and rdbms storage?
Same thing for the line below.

marwan-at-work · 2018-08-20T21:33:14Z

.travis.yml

@@ -7,8 +7,8 @@ go:
 env:
  global:
    - PATH=${PATH}:./bin
-    - POP_PATH=$PWD/cmd/proxy
-    - GO_ENV=test_postgres
+    - POP_PATH=$PWD/cmd/olympus


do we need POP_PATH at all?

yes! because we still have the CDN rdbms driver which is dependent on Pop. So, I made sure that the proxy is independent of Pop but olympus is not.
Once, we have voted on CDN to be kicked out of olympus, the next step would be to create a PR that basically removes POP_PATH from travis

@manugupt1 so say we keep CDN, do we still need RDBMS in the CDN? can we not remove RDBMS and use another storage for CDN?

You are right on!

So, if we decide not to support the RDBMS driver for CDN, we do not need it at all.
For example,
https://github.com/gomods/athens/blob/master/pkg/cdn/metadata/cdn_metadata_entry.go#L14-L19

Supports both mongo and SQL based tags!
If we remove the rdbms driver that stores the metadata https://github.com/gomods/athens/tree/master/pkg/cdn/metadata/rdbms

Then, we can remove all the dependency.

I believe we decided to remove RDBMS from anything Athens related, but we can do it in another PR. @arschles correct me if im wrong :)

Oh cool! I was not aware of of that! Let us wait for @arschles to confirm.

I do believe that RDBMS in CDN is without use. there are no relationships there so any document db will do just fine. Let's get rid of it

Feel free to address it in this or a separate PR @manugupt1

marwan-at-work · 2018-08-20T21:33:38Z

Makefile

@@ -74,3 +73,4 @@ down:
 dev-teardown:
 	docker-compose -p athensdev down
 	docker volume prune
+	docker network prune


this should already happen with docker-compose -p athensdev down -- if not, let's do it in another PR :)

marwan-at-work

🥇

manugupt1 · 2018-08-21T11:34:12Z

Perfect! I will update it later tonight Cheers Manu

________________________________ From: Michal Pristas <notifications@github.com> Sent: Tuesday, August 21, 2018 3:55:29 AM To: gomods/athens Cc: Manu Gupta; Mention Subject: Re: [gomods/athens] Remove pop from Proxy & RDBMS storage getter (#511) @michalpristas commented on this pull request.

________________________________ In .travis.yml<#511 (comment)>:

@@ -7,8 +7,8 @@ go:

env: global: - PATH=${PATH}:./bin - - POP_PATH=$PWD/cmd/proxy - - GO_ENV=test_postgres + - POP_PATH=$PWD/cmd/olympus I do believe that RDBMS in CDN is without use. there are no relationships there so any document db will do just fine. Let's get rid of it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#511 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAYLj54gaGQ67U3E3C3y16VqnvcRe8bbks5uS7zxgaJpZM4WCVBv>.

michalpristas · 2018-08-21T13:36:33Z

@rchakra3 are your comments addressed?

rohancme · 2018-08-21T18:45:00Z

Based on the comments here I'm assuming we'll get rid of olympus/models entirely with the follow up PR to completely remove the rdbms driver anyway?
If so LGTM @michalpristas!

manugupt1 · 2018-08-21T21:42:40Z

Yep! @rchakra3

manugupt1 and others added 8 commits August 16, 2018 20:55

Remove pop from models for now

57314df

Remove rdbms storage from pkg/storage

9f09cd7

Completely remove pop

1777208

update to point to test instance

49b8e6d

Update databasy.yml for travis??

6cef1fc

Merge remote-tracking branch 'origin/remove-rdbms' into remove-pop-rdbms

2c59093

Remove database.yml

1dc9123

manugupt1 changed the title ~~Remove pop & rdbms~~ [WIP] Remove pop & rdbms Aug 17, 2018

manugupt1 added 2 commits August 17, 2018 19:46

Do not use db migrations and creation

0928c47

Remove pop dependency from cdn

fe9e55c

manugupt1 mentioned this pull request Aug 18, 2018

'buffalo test' requires a database, can we remove it? #450

Closed

manugupt1 added 11 commits August 18, 2018 13:21

add schema for olympus assuming cdn driver is part of olympus

6d3098a

Move CI/CD and test scripts to point pop to olympus

6dd18fb

Update database.yml and keep it only for olympus

aec759d

Add migrations to travis

f1f01fc

Add debug logs to see what travis is doing

384eaea

Update travis to use olympus database.yml

e2046f1

Moarrrr logs to see where travis is pointint the db towards

414f4ac

Change env to test

aa47460

Use test as the default environment if not specified otherwise

c203fed

Check if test new rdbms storage succeeds

3884e17

Try to see which connection string rdbms test suite is using on travis

1054f41

manugupt1 force-pushed the remove-pop-rdbms branch from 951be21 to 1054f41 Compare August 18, 2018 18:41

Update db tags for cdn driver

0023831

manugupt1 added 2 commits August 18, 2018 14:55

create db by using travis go_env

4349ae2

Remove sql from database config as it is removed from docker-compose

4e036c8

manugupt1 force-pushed the remove-pop-rdbms branch from 96f2939 to 4e036c8 Compare August 18, 2018 19:03

Remove extra logs that I missed

6289fed

manugupt1 changed the title ~~[WIP] Remove pop & rdbms~~ Remove pop from Proxy & RDBMS storage getter Aug 18, 2018

manugupt1 added 2 commits August 18, 2018 15:18

Merge branch 'master' into remove-pop-rdbms

2974ae8

Add newline

1f57cdb

rohancme mentioned this pull request Aug 19, 2018

Plumb config #517

Closed

rohancme reviewed Aug 19, 2018

View reviewed changes

Merge branch 'master' into remove-pop-rdbms

3add1e4

This was referenced Aug 20, 2018

[WIP] Comment pop & db connection from models #497

Closed

[WIP]Remove rdbms storage from pkg/storage #502

Closed

michalpristas reviewed Aug 20, 2018

View reviewed changes

Merge branch 'master' into remove-pop-rdbms

04a471e

michalpristas reviewed Aug 20, 2018

View reviewed changes

Merge branch 'master' into remove-pop-rdbms

9e69dfc

remove models.go and test file from proxy

c1921a1

marwan-at-work requested changes Aug 20, 2018

View reviewed changes

removing extraneous network prune from Makefile

bdf8211

marwan-at-work approved these changes Aug 20, 2018

View reviewed changes

michalpristas approved these changes Aug 21, 2018

View reviewed changes

rohancme approved these changes Aug 21, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into remove-pop-rdbms

97ea321

manugupt1 mentioned this pull request Aug 21, 2018

put git and go in runtime image #546

Closed

marwan-at-work merged commit c0635b7 into gomods:master Aug 21, 2018

marpio mentioned this pull request Aug 23, 2018

Ensure Storage Streaming #383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pop from Proxy & RDBMS storage getter #511

Remove pop from Proxy & RDBMS storage getter #511

manugupt1 commented Aug 17, 2018 •

edited

Loading

codecov-io commented Aug 18, 2018 •

edited

Loading

rohancme Aug 19, 2018

manugupt1 Aug 20, 2018

michalpristas Aug 20, 2018

manugupt1 Aug 20, 2018

michalpristas Aug 20, 2018

michalpristas Aug 20, 2018

arschles commented Aug 20, 2018

marwan-at-work left a comment

marwan-at-work Aug 20, 2018

manugupt1 Aug 20, 2018

marwan-at-work Aug 20, 2018

manugupt1 Aug 20, 2018

marwan-at-work Aug 20, 2018

manugupt1 Aug 20, 2018

marwan-at-work Aug 20, 2018

manugupt1 Aug 20, 2018

michalpristas Aug 21, 2018

michalpristas Aug 21, 2018

marwan-at-work Aug 20, 2018

marwan-at-work left a comment

manugupt1 commented Aug 21, 2018 via email

michalpristas commented Aug 21, 2018

rohancme commented Aug 21, 2018 •

edited

Loading

manugupt1 commented Aug 21, 2018

Remove pop from Proxy & RDBMS storage getter #511

Remove pop from Proxy & RDBMS storage getter #511

Conversation

manugupt1 commented Aug 17, 2018 • edited Loading

codecov-io commented Aug 18, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Aug 20, 2018

marwan-at-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marwan-at-work left a comment

Choose a reason for hiding this comment

manugupt1 commented Aug 21, 2018 via email

michalpristas commented Aug 21, 2018

rohancme commented Aug 21, 2018 • edited Loading

manugupt1 commented Aug 21, 2018

manugupt1 commented Aug 17, 2018 •

edited

Loading

codecov-io commented Aug 18, 2018 •

edited

Loading

rohancme commented Aug 21, 2018 •

edited

Loading