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

Add new parameter to prevent grub-mkconfig being run #59

Closed
wants to merge 4 commits into from

Conversation

cedws
Copy link

@cedws cedws commented May 29, 2020

Closes #47.
I'm afraid I don't really know Ruby tooling so I am not sure if the tests pass.

@puppet-community-rangefinder
Copy link

grub_user is a type

Breaking changes to this file MAY impact these 3 modules (near match):

kernel_parameter is a type

Breaking changes to this file MAY impact these 14 modules (near match):

This module is declared in 6 of 575 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@coveralls
Copy link

coveralls commented May 29, 2020

Coverage Status

Coverage increased (+0.2%) to 94.118% when pulling 0405301 on cedws:OptionalMkconfig into 082bf14 on hercules-team:master.

lib/puppet/type/grub_user.rb Outdated Show resolved Hide resolved
lib/puppet/type/kernel_parameter.rb Outdated Show resolved Hide resolved
lib/puppet/provider/grub_config/grub2.rb Outdated Show resolved Hide resolved
lib/puppet/provider/grub_user/grub2.rb Outdated Show resolved Hide resolved
@raphink
Copy link
Member

raphink commented Jun 4, 2020

This looks good. Now it only needs some tests 😄

Something similar to https://github.com/hercules-team/augeasproviders_grub/blob/master/spec/unit/puppet/provider/kernel_parameter/grub2_spec.rb#L73, but it should expect to never run when run_mkconfig => false.

@raphink raphink added needs-tests enhancement New feature or request labels Jun 4, 2020
@cedws
Copy link
Author

cedws commented Jul 19, 2020

Apologies for the delay on this. I had a look at writing some tests but am having some toolchain issues. I've installed the dependencies with bundler and am trying to run the tests with rspec, but it's spitting out some errors. Any ideas?

@raphink
Copy link
Member

raphink commented Jul 20, 2020

Are you running bundle exec rake spec?

@cedws
Copy link
Author

cedws commented Jul 21, 2020

Ah, no, I was just running rspec from GEMPATH, thanks. I've just committed a single test, is this along the right lines? I assume you would like me to add a test for the other types as well (grub_config, grub_user, et al)?

@raphink
Copy link
Member

raphink commented Jul 22, 2020

Yes, please!

@cedws
Copy link
Author

cedws commented Aug 2, 2020

Sorry, not getting very far with this. Tried running bundle exec rake acceptance on Ruby 2.7 but get this. Then I saw CI might be using 2.4.4 (though it doesn't appear to actually run the acceptance tests?) so I downgraded, now bundle install is failing because it looks like something to do with Augeas is failing to compile. I have the headers installed on my system.
Also tried doing this all in Docker but I guess the acceptance tests require virtualisation so that won't work.
I do need to add acceptance tests for the run_mkconfig parameter for the grub_menuentry/grub_user/... types correct?

@raphink
Copy link
Member

raphink commented Aug 3, 2020

There's no acceptance tests in this module, all the tests are declared as unit tests, so you run all the tests with bundle exec rake spec.

@trevor-vaughan
Copy link
Contributor

There actually are acceptance tests and it would be great to run them. The CI can't run them because they require virtual machines and are run through beaker.

I suggest using PDK to set up an isolated dev environment or RVM or rbenv but not system Ruby.

@raphink
Copy link
Member

raphink commented Aug 3, 2020

Oh my bad, I didn't think we had any acceptance tests in augeasproviders modules. Sorry about that.

And I'm guessing docker containers would not be sufficient for grub tests.

@trevor-vaughan
Copy link
Contributor

Nope, docker won't work at all for GRUB. Need the real deal :-D.

I had to add them due to the complexity of GRUB itself and just being too dangerous to get wrong.

@raphink
Copy link
Member

raphink commented Aug 3, 2020

We used to run beaker on Travis with openstack or AWS nodes, but it's a paid solution.

@trevor-vaughan
Copy link
Contributor

Yeah, that's the issue. Without someone to put up a system to run the tests, there's not much that can be done besides run them by hand.

@trevor-vaughan
Copy link
Contributor

I'll be able to review this next week if it can wait that long.

@trevor-vaughan
Copy link
Contributor

I'm getting failures in beaker with the code that I'll dig into tomorrow.

Copy link
Contributor

@trevor-vaughan trevor-vaughan left a comment

Choose a reason for hiding this comment

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

After running the acceptance tests, it looks like the :run_mkconfig parameter also needs to be added to the grub_menuentry and grub_user types.

Without this, you will end up with Invalid parameter run_mkconfig(:run_mkconfig)

@trevor-vaughan
Copy link
Contributor

@cedws Let us know if you need help with the testing. Things are 90% there!

@cedws
Copy link
Author

cedws commented Aug 14, 2020

@trevor-vaughan Thanks very much for this. I will not be able to get to it for a week or so.

@jcpunk
Copy link

jcpunk commented Oct 19, 2020

I'll confess interest in this patch...

@raphink
Copy link
Member

raphink commented Oct 22, 2020

@cedws are you still interested in working on this?

@cedws
Copy link
Author

cedws commented Oct 22, 2020

I am, but have quite a lot of things going on at the moment. It's still on my to-do list. Anybody is welcome to pick this up.

@vox-pupuli-tasks
Copy link

Dear @cedws, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@cedws cedws closed this Jun 21, 2022
@cedws cedws deleted the OptionalMkconfig branch June 21, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can the call to grub2-mkconfig be disableable?
5 participants