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

Go modules support for 1.12 #297

Merged
merged 4 commits into from
Apr 25, 2019
Merged

Conversation

azihsoyn
Copy link
Contributor

This PR adds support for running this tool against Go modules.

Closes #234

@azihsoyn
Copy link
Contributor Author

I'll fix CI using build tag for old version compatibility.

Thanks.

Copy link
Member

@gcmurphy gcmurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 🎉 💯

@azihsoyn first of all. Thank you SO MUCH! for taking the time to submit this PR. It has been the most requested feature, and something I've not had the time to get around to. At a first glance this seems really great.

My only asks would be:

  1. Can you clarify what this change would mean for Go version support? I'm happy to move with the latest but we will need to update docs / CI jobs accordingly.

  2. Can you please replace the commented out tests to use Skip() in the body instead? I'd like to keep that showing in test output if possible as a reminder to revisit / re-evaluate them.

@ccojocar do you have any additional feedback on this?

analyzer_test.go Outdated Show resolved Hide resolved
analyzer_test.go Outdated Show resolved Hide resolved
analyzer_test.go Outdated Show resolved Hide resolved
@azihsoyn azihsoyn changed the title Go modules support Go modules support for 1.12 Apr 20, 2019
@azihsoyn
Copy link
Contributor Author

Hi, @gcmurphy! Thanks for the review!

I'll fix your pointed places ASAP😄

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! It would be great if we could fix a few things to get the tests passing. I believe we can retrieve the parsing error from the package.

analyzer.go Outdated Show resolved Hide resolved
analyzer.go Outdated Show resolved Hide resolved
analyzer.go Show resolved Hide resolved
analyzer_test.go Outdated Show resolved Hide resolved
analyzer_test.go Outdated Show resolved Hide resolved
analyzer_test.go Outdated Show resolved Hide resolved
@azihsoyn
Copy link
Contributor Author

Hi, @gcmurphy, @ccojocar!

I fixed for all test passes.

But packages library may not be supported for 1.10 or older.

Can I remove test config from .travis.yml ?

@ccojocar
Copy link
Member

Thanks @azihsoyn! It looks great!

Can I remove test config from .travis.yml ?

Yes, Let's remove the 1.9 and 1.10 from travis config. I think that we will bump the version to 2.0.0 on the next release to make it clear that is a breaking change.

@codecov-io
Copy link

codecov-io commented Apr 22, 2019

Codecov Report

Merging #297 into master will not change coverage.
The diff coverage is 88.23%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #297   +/-   ##
======================================
  Coverage    56.3%   56.3%           
======================================
  Files           9       9           
  Lines         492     492           
======================================
  Hits          277     277           
  Misses        188     188           
  Partials       27      27
Impacted Files Coverage Δ
analyzer.go 82.4% <88.23%> (ø) ⬆️

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 eaba99d...4856018. Read the comment docs.

@ccojocar
Copy link
Member

@gcmurphy Please could you have a look over this PR? I'm waiting for your thumb up before merging it. Thanks

@ccojocar ccojocar merged commit 85d1808 into securego:master Apr 25, 2019
@azihsoyn azihsoyn deleted the feature/for_gomodule branch April 25, 2019 09:26
@azihsoyn
Copy link
Contributor Author

Thanks for review and merging!

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.

Go Modules support
4 participants