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

Fixes #27897 - Parameter type is not required #7054

Merged
merged 3 commits into from Oct 6, 2019

Conversation

ofedoren
Copy link
Member

As I mentioned in the issue, it causes backward incompatibility (at least in the hammer, but there could be a problem in the custom scripts that use API).

@theforeman-bot
Copy link
Member

Issues: #27897

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

cad1fc3 is the commit in question that added it via #6965. I agree it shouldn't be required. Prior to the commit it was a string and for API backwards compatibility this should remain.

Could you add a test that verifies the type is a string when not passed in to prevent future regressions?

@ofedoren
Copy link
Member Author

@ekohl, I added the test, but when was looking at it in debug I saw that without this change

def set_default_key_type
self.key_type ||= Parameter::KEY_TYPES.first
end
it defaults to nil not "string".

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

LGTM but I'd prefer someone with more rails knowledge to give it a look.

@@ -236,6 +236,21 @@ def valid_attrs_with_root(extra_attrs = {})
assert_equal 42, show_response['parameters'].first['value'].to_i
assert_equal 'integer', show_response['parameters'].first['parameter_type']
end

test "should create a group parameter with default parameter type" do
Copy link
Member

Choose a reason for hiding this comment

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

This this test also assert that it was created with the string type?

Copy link
Member Author

Choose a reason for hiding this comment

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

This test just asserts that creating doesn't fail if no parameter_type was provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

The following test does check if there is no parameter type was provided then it defaults to "string"

assert_equal 'string', show_response['parameters'].first['parameter_type']

@ofedoren
Copy link
Member Author

[test foreman]

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @ofedoren !

@tbrisker tbrisker merged commit 0359488 into theforeman:develop Oct 6, 2019
@ofedoren ofedoren deleted the bug-27897-param-type-required branch July 12, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants