-
Notifications
You must be signed in to change notification settings - Fork 60
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
Azure labels and tag mapping support for a new refresh #229
Conversation
@AlexanderZagaynov - the BZ for this PR has a target release of 5.8.4 (Fine branch) and this PR is specific to the Graph refresh which is not supported in the Fine release, so this PR cannot be backported. You will have to make a separate PR for the Fine branch to add tagging support for the old refresh. Getting this into the Fine branch is the priority. |
cc6b2e4
to
43e0441
Compare
@@ -60,6 +60,54 @@ def key_pairs(extra_attributes = {}) | |||
super(attributes.merge!(extra_attributes)) | |||
end | |||
|
|||
def vm_and_template_labels(extra_attributes = {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ladas @juliancheal these collections are starting to pile up in multiple providers, do you see a way to share them?
(I'm fine with duplicating for gaprindashvili & fine, then refactoring)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify, I'm not sure sharing vm_and_template_labels/taggings between 2 VM providers is a big win;
I'm more thinking of a "polymorphic" anything-custom-attributes, anything-taggings, something like these:
https://github.com/ManageIQ/manageiq-providers-kubernetes/blob/5ec31943f8/app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb#L359-L400
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we should probably try to unify these next, from what I saw, each provider had it slightly different though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sounds good to me.
@miq-bot add_label blocker |
@AlexanderZagaynov do you have an update on the status of this PR. |
43e0441
to
7e93c0a
Compare
Looks great 👍 I don't remember what you decided about 'Vm' type - do you need separate 'AzureVm', 'AmazonVm' types? IMHO better to get just Vm working & merged first, leave sub-types to a follow-up if desired. |
hi @AlexanderZagaynov - Graph refresh was not backported to the Fine branch, so this PR cannot be backported to Fine either. I see you closed the Fine specific implementation #231 |
https://travis-ci.org/ManageIQ/manageiq-providers-azure/jobs/364111457#L1958 failing because of ManageIQ/manageiq#17212 not merged yet |
04d3085
to
b8e0f61
Compare
@AlexanderZagaynov can you add the gaprindashvili/yes and the fine/yes if this can be backported. |
@@ -279,6 +283,8 @@ def parse_instance(instance) | |||
:orchestration_stack => @data_index.fetch_path(:orchestration_stacks, @resource_to_stack[uid]), | |||
:availability_zone => @data_index.fetch_path(:availability_zones, 'default'), | |||
:resource_group => @data_index.fetch_path(:resource_groups, rg_ems_ref), | |||
:labels => labels, | |||
:tags => map_labels('Vm', labels), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I think this should be 'VmAzure' like in #231, this is based on top of older version of BZ-1493041-old-refresh branch...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to rebase and add some changes to this PR after #231 will be merged
This pull request is not mergeable. Please rebase and repush. |
659cf41
to
3efabb6
Compare
3efabb6
to
bf13604
Compare
bf13604
to
915fda1
Compare
Checked commits AlexanderZagaynov/manageiq-providers-azure@61ea9f4~...915fda1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
@miq-bot remove-label unmergeable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Awesome work @AlexanderZagaynov
Azure labels and tag mapping support for a new refresh (cherry picked from commit a1e134c) https://bugzilla.redhat.com/show_bug.cgi?id=1568807
Gaprindashvili backport details:
|
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1493041
related old refresh type PR: #231
related core repo PR: ManageIQ/manageiq#17212
related UI repo PR: ManageIQ/manageiq-ui-classic#3697