Skip to content

[FIX] Simplify dep fetching #1062

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

Merged
merged 2 commits into from
Feb 11, 2018
Merged

[FIX] Simplify dep fetching #1062

merged 2 commits into from
Feb 11, 2018

Conversation

im-kulikov
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Feb 10, 2018

Codecov Report

Merging #1062 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1062   +/-   ##
=======================================
  Coverage   78.92%   78.92%           
=======================================
  Files          27       27           
  Lines        1879     1879           
=======================================
  Hits         1483     1483           
  Misses        282      282           
  Partials      114      114

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 fad19bf...bd67cc8. Read the comment docs.

@im-kulikov
Copy link
Contributor Author

@vishr any updates?

@vishr vishr merged commit e9f6780 into labstack:master Feb 11, 2018
@im-kulikov im-kulikov deleted the simplify-dep-fetching branch February 11, 2018 15:36
@vishr
Copy link
Member

vishr commented Feb 11, 2018

@im-kulikov I was curious to know what is the reasoning for fetching a specific version for dep, is it creating a problem? The issue I see with this approach is maintenance, with every new release of dep.

@im-kulikov
Copy link
Contributor Author

This is required to commit a specific version.
Otherwise, it is likely to get a dev build with bugs and errors

@vishr
Copy link
Member

vishr commented Feb 11, 2018

Fair enough.

@alexaandru
Copy link
Contributor

@vishr This simplification assumes that everyone is using Windows, which is not the case. Going further, Windows and OSX users will need to handle deps themselves, which seems like a regression.

@im-kulikov
Copy link
Contributor Author

@alexaandru explain?
Makefile is available for unix-like systems..

vishr pushed a commit that referenced this pull request Mar 12, 2018
Enhancements:
    Implemented Response#After()
    Dynamically add/remove proxy targets
    Rewrite rules for proxy middleware
    Add ability to extract key from a form field
    Implemented rewrite middleware
    Adds a separate flag for the 'http/https server started on' message (#1043)
    Applied a little DRY to the redirect middleware (#1053) and tests (#1055)
    Simplify dep fetching (#1062)
    Add custom time stamp format #1046 (#1066)
    Update property name & default value & comment about custom logger
    Add X-Requested-With header constant
    Return error of context.File in c.contentDisposition
    Updated deps
    Updated README.md

Fixes:

    Fixed Response#Before()
    Fixed #990
    Fix href url from armor to echo (#1051)
    Fixed #1054
    Fixed #1052, dropped param alias feature
    Avoid redirect loop in HTTPSRedirect middleware (#1058)
    Fix deferring body close in middleware/compress test (#1063)
    Cleanup code (#1061)
    FIX - We must close gzip.Reader, only if no error (#1069)
    Fix formatting (#1071)
    Can be a fix for auto TLS
@alexaandru
Copy link
Contributor

@im-kulikov OSX is unix-like, and so is Windows with Cygwin installed. Neither of them will be able to run dep-linux-amd64 binary though.

@im-kulikov
Copy link
Contributor Author

@alexaandru oh.. I understand..

@im-kulikov
Copy link
Contributor Author

I can make research and fix this

@alexaandru
Copy link
Contributor

It seems that if we just use the provided install instructions, that is cross platform friendly:

curl https://raw.githubusercontent.com/golang/dep/master/install.sh | sh

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.

3 participants