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

(FM-8611) Reinstall module #209

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

florindragos
Copy link
Contributor

No description provided.

@florindragos
Copy link
Contributor Author

Depends on #208

@codecov-io
Copy link

codecov-io commented Oct 28, 2019

Codecov Report

Merging #209 into master will decrease coverage by 0.12%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #209      +/-   ##
=========================================
- Coverage   59.62%   59.5%   -0.13%     
=========================================
  Files           6       6              
  Lines         535     563      +28     
=========================================
+ Hits          319     335      +16     
- Misses        216     228      +12
Impacted Files Coverage Δ
lib/puppet_litmus/rake_helper.rb 72.32% <100%> (+3.32%) ⬆️
lib/puppet_litmus/rake_tasks.rb 34.56% <25%> (-0.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6b98f3...a357548. Read the comment docs.

@florindragos florindragos force-pushed the reinstall-module branch 2 times, most recently from 09d79f6 to 2b528d9 Compare October 28, 2019 13:58
@michaeltlombardi
Copy link
Contributor

Should this be rebased on master now that the rake task refactor is merged, @florindragos?

@florindragos
Copy link
Contributor Author

Should this be rebased on master now that the rake task refactor is merged, @florindragos?

done

Copy link
Contributor

@michaeltlombardi michaeltlombardi left a comment

Choose a reason for hiding this comment

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

Minor implementation question, otherwise LGTM!

@@ -184,4 +184,21 @@ def install_module(inventory_hash, target_node_name, module_tar)
install_module_command = "puppet module install /tmp/#{File.basename(module_tar)}"
run_command(install_module_command, target_nodes, config: nil, inventory: inventory_hash)
end

def uninstall_module(inventory_hash, target_node_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be more generically able to uninstall any given module? Then we could specify metadata_module_name to remove that one or any of the dependencies or other modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could default to uninstalling the working directory's module, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that but I'm not sure what the use case for this would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Uninstalling dependencies to check behavior is the first thing that comes to mind.

Copy link
Contributor Author

@florindragos florindragos Nov 1, 2019

Choose a reason for hiding this comment

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

Added a a parameter for the module name but if we want to uninstall dependencies, we need to use --force

Copy link
Contributor

@sanfrancrisko sanfrancrisko left a comment

Choose a reason for hiding this comment

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

Minor typo spotted, otherwise LGTM

# Reinstall the puppet module under test on a collection of nodes
#
# @param :target_node_name [Array] nodes on which to install a puppet module for testing.
desc 'reinstall_module - reinstgall module'
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in reinstgall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch

@lionce lionce merged commit 095c5c9 into puppetlabs:master Nov 5, 2019
@florindragos florindragos deleted the reinstall-module branch November 5, 2019 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants