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 MiqServer#server_role_names= #19327

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

agrare
Copy link
Member

@agrare agrare commented Sep 24, 2019

There is a #role and a #role= method that takes a comma separated
list, then a #server_role_names which takes an array but no
#server_roles_names=.

@agrare
Copy link
Member Author

agrare commented Sep 24, 2019

@jrafanie planning on using this for #19328

@jrafanie
Copy link
Member

This makes sense. I don't know why we didn't have this before. I don't know how we can test this. Is it worth expecting assign_role and deactivate_roles get called?

@agrare
Copy link
Member Author

agrare commented Sep 24, 2019

@jrafanie yeah I did check that this path was tested by spec/models/miq_server/role_management_spec.rb by the role= context since that goes through server_role_names= now.

We could add a context similar to role= but instead of testing the comma separated list, test with arrays

@agrare
Copy link
Member Author

agrare commented Sep 25, 2019

@jrafanie added specs for server_role_names= specifically (mostly copied from the role= specs)

@agrare agrare force-pushed the add_miq_server_server_role_names_eq branch from 743a0bf to 86f648b Compare September 25, 2019 12:51
There is a `#role` and a `#role=` method that takes a comma separated
list, then a `#server_role_names` which takes an array but no
`#server_roles_names=`.
@agrare agrare force-pushed the add_miq_server_server_role_names_eq branch from 86f648b to c686ae4 Compare September 25, 2019 17:02
@miq-bot
Copy link
Member

miq-bot commented Sep 25, 2019

Checked commits agrare/manageiq@c57905f~...c686ae4 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 3 offenses detected

app/models/miq_server/role_management.rb

@jrafanie
Copy link
Member

that seems like a rubocop bug in parsing the code. Interesting. I wonder if that long line comment is messing it up.

@jrafanie
Copy link
Member

I like this, only have a minor comment about sorting but LGTM so far.

@jrafanie jrafanie added this to the Sprint 121 Ending Sep 30, 2019 milestone Sep 25, 2019
@jrafanie jrafanie merged commit eec18b9 into ManageIQ:master Sep 25, 2019
@agrare agrare deleted the add_miq_server_server_role_names_eq branch September 25, 2019 18:32
@agrare
Copy link
Member Author

agrare commented Sep 25, 2019

that seems like a rubocop bug in parsing the code. Interesting. I wonder if that long line comment is messing it up.

Yeah, I was tempted to try return role just to see if it'd also complain about a pointless return 😆

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.

3 participants