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

Parse the name of EBS volume or snapshot #121

Merged
merged 1 commit into from
Jan 31, 2017

Conversation

gberginc
Copy link
Contributor

@gberginc gberginc commented Jan 26, 2017

Name is defined as a tag and can be optional. Patch uses the id of
the volume/snapshot if name is not given.

@gberginc gberginc force-pushed the parse_volume_snapshot_name branch from dde57d2 to 93a2732 Compare January 26, 2017 21:54
@gberginc
Copy link
Contributor Author

@miq-bot add_label euwe/no, enhancement

@bronaghs
Copy link

@miq-bot assign @bronaghs

Copy link

@bronaghs bronaghs left a comment

Choose a reason for hiding this comment

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

@gberginc - The code itself looks good but I think we need a spec test for this.

def get_from_tags(resource, item)
(resource['tags'] || []).detect { |tag, _| tag['key'].downcase == item.to_s.downcase }.try(:[], 'value')
end

Choose a reason for hiding this comment

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

Once this PR is merged, the get_from_tags method will be overridden in 3 different RefreshParserInventoryObject subclasses (NetworkManager, CloudManager, StorageManager) - each one identical.
For the record, as the comment on line 62 indicates, the above implementation will replace the current version in the RefreshHelperMethods module when the RefreshParser class becomes obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It hurt very much copying it once more. I was thinking about using a different name. For example, I thought about renaming the existing get_from_tags in the helpers module and using this one instead, but just couldn't find a proper name (get_from_tags_legacy or get_from_tags_refresh_parser didn't sound ok to me).

Any idea?

Choose a reason for hiding this comment

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

@gberginc - Haha I completely know what you mean. If the RefreshParser wasn't on the cusp of being deprecated i would like to see it addressed, but refactoring this disrupts a number of classes and I'm not sure it is worth the effort when it will be addressed properly for the G release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it sucks, but it should be an easy cleanup later :-)

@gberginc gberginc force-pushed the parse_volume_snapshot_name branch from 93a2732 to 4257205 Compare January 30, 2017 03:40
@gberginc
Copy link
Contributor Author

@bronaghs Updated the stubs with an additional tag for the name for both volumes and snapshots. I'll be updating another specs once I get the VCRs for #122.

Name is defined as a tag and can be optional. Patch uses the id of
the volume/snapshot if name is not given.

Signed-off-by: Gregor Berginc <[email protected]>
@gberginc gberginc force-pushed the parse_volume_snapshot_name branch from 4257205 to 774e479 Compare January 30, 2017 05:25
@miq-bot
Copy link
Member

miq-bot commented Jan 30, 2017

Checked commit gberginc@774e479 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🍰

@bronaghs
Copy link

@gberginc - great, thanks.

@gberginc
Copy link
Contributor Author

@bronaghs is this safe to merge or should we wait for the VCR as well?

@chargio
Copy link

chargio commented Jan 31, 2017

@bronaghs @Ladas, is there any other change needed? I see we are dependent on #122, but is that the only thing we need for testing this?

@bronaghs
Copy link

@gberginc - A separate PR using the VCR specs is fine.

@bronaghs bronaghs merged commit 251eafb into ManageIQ:master Jan 31, 2017
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.

5 participants