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

Deprecate dep in favor of Go Modules #100

Merged
merged 5 commits into from
Jan 23, 2019

Conversation

khos2ow
Copy link
Member

@khos2ow khos2ow commented Jan 4, 2019

Prerequisites

Put an x into the box(es) that apply:

  • This pull request fixes a bug.
  • This pull request adds a feature.
  • This pull request enhances existing functionality.
  • This pull request introduces breaking change.

For more information, see the Contributing Guide.

Description

This pull request deprecates dep in favor of Go Modules.

Issues Resolved

List any existing issues this pull request resolves.

Checklist

Put an x into all boxes that apply:

Tests

  • I have added tests to cover my changes.
  • All tests pass when I run make test.

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Code Style

  • My code follows the code style of this project.

@metmajer
Copy link
Member

metmajer commented Jan 7, 2019

Hey @khos2ow, happy new year! I'll look into Go Modules and your PR in the next days. Thanks!

@metmajer metmajer self-requested a review January 18, 2019 17:26
@metmajer
Copy link
Member

metmajer commented Jan 18, 2019

@zimbatm have you tested this PR, including a pipeline run, before you gave your approval?

@zimbatm
Copy link

zimbatm commented Jan 18, 2019

@metmajer isn't CircleCI already doing this for us?

@metmajer
Copy link
Member

@zimbatm You are correct. I guess I'm just startled that you're giving your approval to a feature that hasn't been commented by the codebase maintainer.

@zimbatm
Copy link

zimbatm commented Jan 19, 2019

I was just passing by and happened to look at the PR. I don't think my approval has a lot of weight, except maybe nudge the maintainers?

@metmajer
Copy link
Member

@khos2ow I am looking into your PR at the moment, but I am getting errors with the latest version of gometalinter on my development machine. Obviously, tests are green in CircleCI, but I'll investigate and get back shortly.

@metmajer
Copy link
Member

gometalinter is known to produce issues with go modules. I'll open a PR to replace this one with its alternative golangci-lint.

@metmajer
Copy link
Member

@khos2ow golangci-lint has been included with in #103. Please rebase. Otherwise, LGTM!

@metmajer metmajer merged commit f32b321 into terraform-docs:master Jan 23, 2019
@khos2ow
Copy link
Member Author

khos2ow commented Jan 23, 2019

Sorry guys, I was AFK for few days!

I agree with @zimbatm , generally everyone can approve any PR, but the ones that matter at the end will be the maitainers'. And that's why there's a color coding of approval (grey vs green).

@metmajer that's weird. I've tried make lint on both

  • gometalinter version 2.0.11 built from 17a7ffa42374937bfecabfb8d2efbd4db0c26741 on 2018-09-09T06:19:21Z
  • gometalinter version 2.0.12 built from 102ac984005d45456a7e3ae6dc94ebcd95c2bb19 on 2018-12-16T00:57:56Z

and neither failed. but since you already went ahead with #103 it's all good! Thanks.

@metmajer
Copy link
Member

One of gometalinter's plugins failed due to unresolvable package dependencies even after go mod vendor was executed. It appears as if the plugin was looking up those packages in my (then empty) GO_PATH, and not in the vendor directory. Still, this doesn't explain why we didn't see this error in the pipeline, but overall, golangci-lint seems to be the better alternative (and worked fine ootb).

@khos2ow khos2ow deleted the go-modules branch January 23, 2019 17:16
@khos2ow
Copy link
Member Author

khos2ow commented Jan 23, 2019

Hmm, that's strange. I, yet, need to read more about golangci-lint. 👍

@metmajer
Copy link
Member

I also needed some time to understand the value and impact of Go modules. Since this goes in my spare time, things sometimes take a while. In the future, let's share thoughts in a dedicated Issue before filing a PR (see Contribution Guide). Thanks for another great contribution, @khos2ow!

@metmajer
Copy link
Member

metmajer commented Jan 23, 2019

Hey @zimbatm! I really don't mind a nudge. Still, there's a difference between nudging and approving someone's work. While @khos2ow PR has been complete and indeed a valuable contribution to this project, we often see things the other way round here as well. Just be cautious with giving approvals, and if it's simply about a nudge, I'd value if you'd drop me a message instead. 👍

@metmajer
Copy link
Member

@zimbatm By the way, we'd value your contributions if you're inclined. There's a couple of open issues, so feel free to have a look and let me know if you have questions!

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.

None yet

3 participants