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

Cleanup testing and deprecations #334

Merged
merged 10 commits into from Jan 25, 2017

Conversation

shortdudey123
Copy link
Contributor

No description provided.

```
ChefSpec Coverage report generated...

  Total Resources:   93
  Touched Resources: 75
  Touch Coverage:    80.65%

Untouched Resources:

  ruby_block[set_jvm_search_dirs_on_java_8]   cassandra-dse/recipes/datastax.rb:163
  directory[/var/run/cassandra]      cassandra-dse/recipes/datastax.rb:215
  directory[/var/lib/cassandra/data]   cassandra-dse/recipes/datastax.rb:215
  ruby_block[smash < 2.0-attributes]   cassandra-dse/recipes/config.rb:90
  template[/etc/cassandra/conf/jmxremote.access]   cassandra-dse/recipes/config.rb:257
  template[/etc/cassandra/conf/jmxremote.password]   cassandra-dse/recipes/config.rb:267
  ruby_block[smash >= 2.1-attributes]   cassandra-dse/recipes/config.rb:66
  directory[/etc/mycassandra]        cassandra-dse/recipes/datastax.rb:183
  template[/etc/mycassandra/conf/cassandra.yaml]   cassandra-dse/recipes/config.rb:108
  template[/etc/mycassandra/conf/jvm.options]   cassandra-dse/recipes/config.rb:117
  template[/etc/mycassandra/conf/cassandra-env.sh]   cassandra-dse/recipes/config.rb:126
  template[/etc/mycassandra/conf/logback.xml]   cassandra-dse/recipes/config.rb:136
  template[/etc/mycassandra/conf/logback-tools.xml]   cassandra-dse/recipes/config.rb:136
  template[/etc/mycassandra/conf/cassandra-rackdc.properties]   cassandra-dse/recipes/config.rb:146
  template[/etc/mycassandra/conf/cassandra-topology.properties]   cassandra-dse/recipes/config.rb:157
  template[/etc/mycassandra/conf/cassandra-metrics.yaml]   cassandra-dse/recipes/config.rb:184
  template[/etc/mycassandra/conf/jmxremote.access]   cassandra-dse/recipes/config.rb:257
  template[/etc/mycassandra/conf/jmxremote.password]   cassandra-dse/recipes/config.rb:267
```
```
ChefSpec Coverage report generated...

  Total Resources:   44
  Touched Resources: 44
  Touch Coverage:    100.0%
```
```
Finished in 1 minute 55.64 seconds (files took 7.68 seconds to load)
202 examples, 0 failures

ChefSpec Coverage report generated...

  Total Resources:   96
  Touched Resources: 96
  Touch Coverage:    100.0%
```
@michaelklishin
Copy link
Owner

Thanks. Some of the changes go beyond deprecations and "cleanup".

@michaelklishin
Copy link
Owner

I don't mean to criticize but it's the worst kind of pull request a maintainer can receive. It provides no description, no justification of the changes, involves all kinds of unrelated changes, makes claims of "cleaning things up" while changing things that are a matter of personal preference, and assumes that using the latest and greatest (e.g. CentOS 7.0) is the right thing to do.

Copy link
Owner

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

There are many reasonable or acceptable changes, and you seem to be much more up-to-date than I am with Chef testing and cookbook development practices.

However, next time such pull requests won't be accepted. Please change one thing at a time and provide a justification.

@michaelklishin michaelklishin self-assigned this Jan 25, 2017
@michaelklishin michaelklishin merged commit 87574f6 into michaelklishin:master Jan 25, 2017
@shortdudey123
Copy link
Contributor Author

It provides no description, no justification of the changes

See the commit history :)

@shortdudey123 shortdudey123 deleted the cleanup branch January 25, 2017 18:24
@michaelklishin
Copy link
Owner

@shortdudey123 yeah, right. The good old "see commit history, no further explanation necessary" argument. Sorry but it's deeply flawed. Unless you previously had a conversation with the maintainer, providing a brief overview of your changes saves them time and mental energy.

To be fair some of your commits do provide code metrics so it's more useful than average. Let's not get into this because the PR is merged. Thanks for contributing.

@shortdudey123
Copy link
Contributor Author

Most maintainers read the commit history but i will make sure to cover the changes in the description of PRs for your cookbooks in the future. Sorry about that!

Also, one thought is to add a PR template

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

2 participants