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

Plumb config #517

Closed
wants to merge 30 commits into from
Closed

Plumb config #517

wants to merge 30 commits into from

Conversation

rohancme
Copy link
Contributor

This PR is a continuation of #453

It should resolve #190, #284 and to some extent #261. I've plumbed timeouts through wherever implemented.

Files that are deleted in this PR are due to changes being made in #511 and #513 (Removing pop from storage and removing olympus as a storage implementation), so ideally this should be merged only after those changes have been approved and merged.

@codecov-io
Copy link

Codecov Report

Merging #517 into master will increase coverage by 3.83%.
The diff coverage is 56.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage    41.2%   45.03%   +3.83%     
==========================================
  Files          99       97       -2     
  Lines        2830     2782      -48     
==========================================
+ Hits         1166     1253      +87     
+ Misses       1554     1409     -145     
- Partials      110      120      +10
Impacted Files Coverage Δ
pkg/config/cdn.go 0% <0%> (ø)
cmd/olympus/actions/merge_db.go 0% <0%> (ø) ⬆️
pkg/config/timeout.go 0% <0%> (ø)
pkg/storage/gcp/deleter.go 62.5% <100%> (ø) ⬆️
pkg/storage/module/upload.go 97.22% <100%> (ø) ⬆️
pkg/download/goget/goget.go 66.31% <100%> (-0.36%) ⬇️
pkg/storage/gcp/saver.go 62.5% <100%> (ø) ⬆️
pkg/storage/module/delete.go 97.22% <100%> (ø) ⬆️
cmd/proxy/actions/filter.go 75.75% <100%> (ø) ⬆️
cmd/proxy/actions/app_proxy.go 76.92% <100%> (ø) ⬆️
... and 27 more

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 70167dc...013d525. Read the comment docs.

@rohancme
Copy link
Contributor Author

Also resolving conflicts in this PR is going to be SUPER annoying :) I'll probably do a squash and rebase after reviews

@ghost
Copy link

ghost commented Sep 2, 2018

Might i suggest a squash and rebase first? That way we can start clean now that the config file PR is done.

@michalpristas
Copy link
Member

i'm also leaning towards rebase-review order. In case of rebase just ping us directly and we can put priority into reviewing this and not pushing anything else in

@rohancme
Copy link
Contributor Author

rohancme commented Sep 3, 2018

Working on this today. I'll attempt a squash + rebase but there are so many conflicts I might just start afresh.

@rohancme rohancme mentioned this pull request Sep 4, 2018
@rohancme
Copy link
Contributor Author

rohancme commented Sep 4, 2018

@robjloranger @michalpristas closing this in favor of #627 . There were simply too many merge conflicts.

This also forced me to get caught up with the latest changes :)

@rohancme rohancme closed this Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate all environment variable fetching at the top of the stack
4 participants