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

Allow resources without parameters #2

Merged
merged 1 commit into from
May 29, 2015
Merged

Allow resources without parameters #2

merged 1 commit into from
May 29, 2015

Conversation

sspreitzer
Copy link
Contributor

aka default parameters

Please double check.

@rnelson0
Copy link
Owner

@sspreitzer Can you provide a sample of what your hiera looks like for the resource without parameters? I tried this but I receive an error:

---
test2:
  notify:
    'message with no parameters'

  1) hiera_resources should run hiera_resources("test2") and return {"notify"=>"message with no parameters"}
     Failure/Error: ) }
     NoMethodError:
       undefined method `each' for "message with no parameters":String
     # ./spec/fixtures/modules/hiera_resources/lib/puppet/parser/functions/hiera_resources.rb:19:in `block (2 levels) in <top (required)>'
     # ./spec/fixtures/modules/hiera_resources/lib/puppet/parser/functions/hiera_resources.rb:17:in `each'
     # ./spec/fixtures/modules/hiera_resources/lib/puppet/parser/functions/hiera_resources.rb:17:in `block in <top (required)>'
     # ./spec/functions/hiera_resources_spec.rb:14:in `block (2 levels) in <top (required)>'

Perhaps my yaml-fu is weak or I am not imagining your use case properly. Thanks!

@sspreitzer
Copy link
Contributor Author

;-) please try

---
resources:
  notify:
    'message with no parameters':

@sspreitzer
Copy link
Contributor Author

It could be this one would work without the pull request as well:

---
resources:
  notify:
    'my message': {}

@rnelson0
Copy link
Owner

@sspreitzer That does make more sense! The second test case does work without the PR but I do like being able to support the first. Can you rebase against master and add an rspec test as well as supporting hiera data? Thanks!

@sspreitzer
Copy link
Contributor Author

Rebasing done, but to be honest i dont know how to write the rspec. Can you maybe do it?

@sspreitzer
Copy link
Contributor Author

Added push access to sspreitzer/hiera_resources

@rnelson0
Copy link
Owner

There's an rspec test in the file spec/functions/hiera_resources_spec.rb and the hiera data is at spec/fixtures/hieradata/default.yaml. You can add another test case to the .rb file. At the top of the repo, run rake spec_prep once to have the fixtures installed, then rake spec_standalone to run tests. The test should fail at first, then you can add your example hiera to default.yaml and run rake spec_standalone again. If the test and data match, you are good to go!

I can make the change if you'd really like but I thought you might be interested in learning. Let me know.

Also, when rebasing if you can squash the commits down into 1 commit that would be great!

@sspreitzer
Copy link
Contributor Author

Hi there, the commits are pushed, so squashing them is impossible for now. Hence the delta/outcome is the same. Thank you for the outline, i will try to write the test. ;)

Btw. do you know or assume that a pull request must stay updated to upstream? Afaik it can stay offsync as it is a patch that will be applied if OK. So only the delta between branch x and branch y at time z will be applied to branch y at current time. Dealing with the delta between time z and current time is not needed. Am i right?

@daenney
Copy link

daenney commented May 25, 2015

I would suggest taking a refresher course on core git concepts and what you want to do. Even if commits have been pushed out it is entirely possible to rebase and squash them and push the updated result. Git allows you to rewrite any part of history should you feel so inclined. I would not ever do this on master since you'd destroy any person's clone from it but on topic branches this is totally feasible and part of the usual workflow.

As far as syncing with upstream goes. As long as the patch can be applied, it doesn't matter indeed. However, if changes land in upstream that change enough of the code you'll have to rebase. Since @rnelson0 asked for a squash it's not that much of a stretch to rebase your changes too, though indeed currently not necessary.

@rnelson0
Copy link
Owner

@sspreitzer I have taken a stab at explaining a rebase in the past at http://rnelson0.com/2014/12/23/using-git-rebase-to-rewrite-history/. You need to squash/fixup the commits till you get just one. If you have any specific questions, ask.

Note: get the test done before you do the rebase, since you'll need to squash that commit as well.

@sspreitzer
Copy link
Contributor Author

Cool, thank you guys. I think it looks better now with one commit? For the rspec i will come up in some days.

@sspreitzer
Copy link
Contributor Author

So, here we are. Can you please double check?

aka default parameters
plus rspec tests
@rnelson0
Copy link
Owner

Awesome, looks good! I'll try and get this merged later today.

rnelson0 added a commit that referenced this pull request May 29, 2015
Allow resources without parameters
@rnelson0 rnelson0 merged commit 8491b13 into rnelson0:master May 29, 2015
@rnelson0
Copy link
Owner

@sspreitzer Merged, thank you very much!

@sspreitzer
Copy link
Contributor Author

@rnelson0 Thank you, it was a pleasure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants