Skip to content

Fix the argument list for nonstandardpooler #87

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

Merged
merged 1 commit into from
Aug 20, 2020
Merged

Fix the argument list for nonstandardpooler #87

merged 1 commit into from
Aug 20, 2020

Conversation

jarretlavallee
Copy link

This PR fixes the following error in 0.10.0 when using nonstandard_pooler. The ondemand argument was added to the other retrieve methods but missed in nonstandardpooler causing failures when provisioning.

$ floaty get aix-7.2-power --service ns                                                                                       
error: wrong number of arguments (given 7, expected 6). Use --trace to view backtrace

Status

[Ready for Merge]

Description

  • Fix an issue where nonstandard pooler was incompatible with the on demand provisioning changes

Related Issues

Todos

  • Tests
  • Documentation

Reviewers

@puppetlabs/dio
@highb
@briancain

Prior to this commit, 7 arguments would be sent into the `retrieve`
method in nonstandardpooler. There were only 6 arguments in the method
definition so an error would be thrown. This commit adds the `ondemand`
argument to the `retrieve` method, but does not utilize it.
@jarretlavallee jarretlavallee requested review from briancain, highb and a team as code owners August 13, 2020 18:44
Copy link
Contributor

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Good catch, I think we must also have to update this for the non-standard pooler too?

def self.retrieve(verbose, os_type, token, url, _user, _options)

@jarretlavallee
Copy link
Author

@briancain I think that is the same line this PR is for. Was there another place I should be adding this?

@briancain
Copy link
Contributor

Lol oops you're right 🤦 I just did a qiuck search to see if there were any other places that could of been missed 😅 Alright makes sense! 😄

@mattkirby mattkirby merged commit 13fee7e into puppetlabs:master Aug 20, 2020
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.

3 participants