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

Use caching to improve spec performance #136

Merged
merged 4 commits into from
Feb 14, 2017

Conversation

ncs-alane
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a09e655 on ncs-alane:ncs-alane-test-caching into 720c6bc on dev-sec:master.

@atomic111
Copy link
Member

@ncs-alane can you please rebase your pr

@artem-sidorenko
Copy link
Member

artem-sidorenko commented Feb 8, 2017

We can take the cached part of chef, but I'm not sure how we can proceed with is_expected refactoring. It makes sense and looks nice, and I personally like it, however I would like to have only one style in all chef cookbooks to keep it simple.

@ncs-alane are you maybe open to provide similar contribution for chef-ssh-hardening ? All other cookbooks (apache, nginx ..) can be updated by myself when I get to them.

@atomic111 @chris-rock what is your view on this?

@ncs-alane ncs-alane force-pushed the ncs-alane-test-caching branch from a09e655 to 52cb3d0 Compare February 9, 2017 04:13
@ncs-alane
Copy link
Contributor Author

@atomic111: done!

@artem-sidorenko: I would be happy to make further contributions! If you prefer, I can split the caching improvements and the RSpec 3 syntax changes in to separate pull requests.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 52cb3d0 on ncs-alane:ncs-alane-test-caching into 554258c on dev-sec:master.

@atomic111
Copy link
Member

@ncs-alane and @artem-sidorenko this would be nice, if we have the same style in all our cookbooks.

Copy link
Member

@artem-sidorenko artem-sidorenko left a comment

Choose a reason for hiding this comment

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

@ncs-alane it looks good to me, see some questions about line breaks and suggestion/idea about spec_helper.

Many thanks for this work! Feel free to open a PR for chef-ssh-hardening and/or other chef hardening cookbooks :)

command: 'chmod go-w -R /usr/local/bin'
)
end

it 'remove write permission from /usr/sbin' do
expect(chef_run).to run_execute('remove write permission from /usr/sbin').with(
is_expected.to run_execute('remove write permission from /usr/sbin').with(
Copy link
Member

Choose a reason for hiding this comment

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

Is there any special reason why the the lines above got spitted to multiple lines and here not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@artem-sidorenko: no special reason. Can we enable the Metrcis/LineLength cop in RuboCop so there is an enforceable rule for when to break lines?

Copy link
Member

Choose a reason for hiding this comment

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

@ncs-alane I did not know/notice its disabled

Yes, feel free to enable it, I usually have it like this

Metrics/LineLength:
  Max: 120

This should be probably Ok, what do you think?

I took a look to the .rubocop.yml in chef-os-hardening and chef-ssh-hardening, maybe it makes sense to go through this file in both repos in a dedicated PRs and see if we can minimize the configured settings and have more defaults from rubocop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 80 characters, which is what I recommend as well; 80 character lines display nicely on GitHub without the need for scrolling. Switching over to Cookstyle also seems like a good way to align cookbook style specifically with the Chef community.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, lets stay with 80 and enabling of LineLength cop

Last time I tried cookstyle - it was in the very early stage, I do not know the current state of it. I think its a good idea to check this way, but a dedicated PR would be better for that from my view (in order to get this PR merged as soon as possible)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've enabled the 80 character line limit, but I limited it to the spec files since there are a number of violations in other files that need to be addressed separately.

@@ -18,20 +18,24 @@
require_relative '../spec_helper'
Copy link
Member

Choose a reason for hiding this comment

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

can we maybe also change all require_relative to require 'spec_helper' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make that change. Alternatively, RSpec will read command line configuration options from .rspec so we can put --require spec_helper there and remove all of the require statements.

Copy link
Member

Choose a reason for hiding this comment

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

@ncs-alane good idea, lets do that

Still if `require 'spec_helper' is a more common way, we do not have anything special in the spec_helper, so that should be fine

Can you please add --color and --format documentation in the same step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


it 'not write log for cpu_vendor fallback' do
is_expected.to_not write_log(
'WARNING: Could not properly determine the cpu vendor. Fallback to ' \
Copy link
Member

Choose a reason for hiding this comment

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

is there any reason for this line break?

- enable color
- use documentation format
- require spec_helper
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ff06d9e on ncs-alane:ncs-alane-test-caching into 554258c on dev-sec:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ce22f71 on ncs-alane:ncs-alane-test-caching into 554258c on dev-sec:master.

@artem-sidorenko artem-sidorenko merged commit b774c1b into dev-sec:master Feb 14, 2017
@artem-sidorenko
Copy link
Member

@ncs-alane many thanks!

@artem-sidorenko
Copy link
Member

artem-sidorenko commented May 10, 2017

@ncs-alane as far I remember you wanted to provide a similar contribution for chef-ssh-hardening. Is this still the case and do you know if this is going to happen in the next time?

@ncs-alane ncs-alane deleted the ncs-alane-test-caching branch May 11, 2017 03:29
@ncs-alane
Copy link
Contributor Author

@artem-sidorenko I definitely intend on making the contribution; I have just been quite busy lately. 😢

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.

4 participants