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.

Sign up for GitHub

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

Jump to bottom

AzureStack events for targeted refresh #588

Merged
merged 1 commit into from
Oct 11, 2019
Merged

AzureStack events for targeted refresh #588

merged 1 commit into from
Oct 11, 2019

Conversation

mancabizjak
Copy link
Contributor

In this commit we add automate class and instances corresponding to AzureStack provider events. All the events added here trigger targeted refresh.

Links

@miq-bot add_label enhancement
@miq-bot assign @gmcculloug

/cc @Ladas @agrare

miha-plesko
miha-plesko approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@miha-plesko miha-plesko left a comment

Choose a reason for hiding this comment

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

LGTM. It's great to see targeted refresh actually invoked upon events.

@gmcculloug gmcculloug requested a review from lfu October 10, 2019 12:05
@gmcculloug
Copy link
Member

@mancabizjak Thanks for the contribution.

One question about the path, where is the naming Azure_Stack coming from? I would expect this to be camel case AzureStack which would be consistent with the other EMS events classes: https://github.com/ManageIQ/manageiq-content/tree/master/content/automate/ManageIQ/System/Event/EmsEvent

Ladas
Ladas approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 looks good (I don't have merge rights)

gmcculloug
gmcculloug suggested changes Oct 10, 2019
View reviewed changes
Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one question about the naming of Azure_Stack (here #588 (comment)) before it can be merged.

@mancabizjak
Copy link
Contributor Author

mancabizjak commented Oct 11, 2019 •
edited
Loading

@gmcculloug I share your concern about the consistency in the naming of Azure_Stack automate class. The underscore case comes from the current implementation of EventParser in AzureStack provider, where we use AZURE_STACK source (see event_parser#L9), which was autogenerated from the provider name.

Renaming the class without updating EventParser in the provider would give us something like this:

[----] I, [2019-10-11T13:07:25.811650 #27587:2b28c779a5fc]  INFO -- : Following Relationship [miqaedb:/System/Event/EmsEvent/AZURE_STACK/Administrative_Microsoft.Compute_virtualMachines_write_Succeeded#create]
[----] E, [2019-10-11T13:07:25.867143 #27587:2b28c779a5fc] ERROR -- : Class [System/Event/EmsEvent/AZURE_STACK] not found in MiqAeDatastore
[----] I, [2019-10-11T13:07:25.867176 #27587:2b28c779a5fc]  INFO -- : Followed  Relationship [miqaedb:/System/Event/EmsEvent/AZURE_STACK/Administrative_Microsoft.Compute_virtualMachines_write_Succeeded#create]

Personally, I would prefer to name the class AzureStack as well, and don't mind making the required change in the provider (@agrare does that make sense)?

@agrare
Copy link
Member

agrare commented Oct 11, 2019

Yeah that works for me, we can make it AZURESTACK, that would be consistent with other providers

@mancabizjak mancabizjak mentioned this pull request Oct 11, 2019
Merged
@mancabizjak
Add AzureStack events trigger targeted refresh
48c5dec 
This commit adds automate class and instances corresponding
to AzureStack provider events. All the events added here
trigger targeted refresh.

Signed-off-by: Manca Bizjak <manca.bizjak@xlab.si>
@mancabizjak mancabizjak force-pushed the add-azurestack-events-for-targeted-refresh branch from 1b6a543 to 48c5dec Compare October 11, 2019 13:02
@miq-bot
Copy link
Member

miq-bot commented Oct 11, 2019

Checked commit xlab-si@48c5dec with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

@mancabizjak
Copy link
Contributor Author

Okay, done - I renamed the automate class to AzureStack and made the change in ManageIQ/manageiq-providers-azure_stack/pull/17. I think we're good to go 👍

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3703

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 96.799%
Totals Coverage Status
Change from base Build 3700: 0.001%
Covered Lines: 2752
Relevant Lines: 2843
💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3703

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 96.799%
Totals Coverage Status
Change from base Build 3700: 0.001%
Covered Lines: 2752
Relevant Lines: 2843
💛 - Coveralls

@gmcculloug gmcculloug merged commit 22a9958 into ManageIQ:master Oct 11, 2019
@gmcculloug gmcculloug added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas approved these changes

@gmcculloug gmcculloug gmcculloug approved these changes

@lfu lfu lfu approved these changes

@miha-plesko miha-plesko miha-plesko approved these changes

@agrare agrare agrare approved these changes

Assignees

@gmcculloug gmcculloug

Labels
changelog/yes enhancement events ivanchuk/no providers/cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mancabizjak @gmcculloug @agrare @miq-bot @coveralls @Ladas @lfu @miha-plesko