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

don't explicitly include puppet-lint #292

Merged
merged 1 commit into from
Jun 27, 2016

Conversation

bastelfreak
Copy link
Member

@bastelfreak bastelfreak commented Jun 24, 2016

Please check the following items before submitting a PR -- thank you!

Note that this project is released with a Contributor Code of Conduct.
By participating in this project you agree to abide by its terms.
Contributor Code of Conduct.

  • There is no existing PR that addresses this problem
  • Mentioned any existing issues in your commit so they get linked and
    closed once this PR gets merged, i.e: Closes #1554 in the body of a commit
  • Followed the instructions in the Contributing document
  • Ran the unit/spec tests and ensured they still pass
  • Added tests to cover the new behaviour
  • Updated the documentation to match the changes
  • When possible, add an entry to the CHANGELOG file
  • Squashed your PR down to a single commit. You may forego this if the PR
    tries to address multiple issues. Though we prefer one PR per feature/fix,
    sometimes that's not feasible. In that case ensure that a single feature/fix
    and associated tests and documentation is bundled up in one commit

Optional, but extra points:

  • Added tests to ensure the old behaviour cannot accidentally be
    reintroduced

we've multiple gems as puppet-lint plugins that all requier puppet-lint

we've multiple gems as puppet-lint plugins that all requier puppet-lint
@roidelapluie
Copy link
Member

I think we should include it excplictely. Maybe not from git anymore.

@roidelapluie
Copy link
Member

👎

@bastelfreak
Copy link
Member Author

Why include it explicitly? We try to keep the Gemfile tight, we also removed rspec and rubocop because they're required by other gems.

@roidelapluie
Copy link
Member

why not? I do NOT approve this PR. It's not just a transitive dependancy, its more than that

@roidelapluie roidelapluie mentioned this pull request Jun 24, 2016
@rnelson0
Copy link
Member

I think we agree that we absolutely should not reference git as the source and that bundler should pull from rubygems latest by default, especially given the issues we ran into earlier this week based on the version bump at HEAD.

If that is the case, and we also include puppet-lint checks, there is no functional benefit to referencing puppet-lint directly. If we do not include puppet-lint checks, then there is a functional benefit, and in fact requirement, to reference puppet-lint directly.

With that in mind, I see no harm right now in removing the dependency on puppet-lint. In the long term, including it would protect us against removing all the check gems and being left without the puppet-lint gem. I don't see us accidentally stepping in that issue anytime soon, but I can see why the explicit declaration does prevent that.

@roidelapluie I think this might be why you are blocking it? If so, would you approve leaving the dependency in place, without a version or a source, so that it pulls rubygems' latest?

@roidelapluie
Copy link
Member

I do think that puppet-lint is more than a transitive dependency and that we should pull rubygems last yes

@roidelapluie roidelapluie merged commit f696e89 into voxpupuli:master Jun 27, 2016
@bastelfreak bastelfreak deleted the fixlint branch June 27, 2016 10:51
@rnelson0
Copy link
Member

@roidelapluie Great, we'll make sure puppet-lint gets pulled in via modulesync in the next revision.

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