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

Adding the option to set default vlan #16504

Merged
merged 1 commit into from
Jan 10, 2018
Merged

Conversation

AlonaKaplan
Copy link
Contributor

@AlonaKaplan AlonaKaplan commented Nov 20, 2017

Adding the option to set default vlan

This change is needed by ManageIQ/manageiq-providers-ovirt#150.
PR ManageIQ/manageiq-providers-ovirt#150 is automatically setting a default 'vlan' on a vm provision. It is done by overriding 'miq_provison_workflow.set_default_vlan' method, introduced in this PR.
Since a default vlan is set, there is no need for the 'vlan' field to be required.

Fixes https://bugzilla.redhat.com/1449157

@gmcculloug
Copy link
Member

@AlonaKaplan I assume you are doing this to override in a provider. Is there a PR in a separate repo that relies on this change that can be linked here.

@AlonaKaplan
Copy link
Contributor Author

needed by ManageIQ/manageiq-providers-ovirt#150

@agrare
Copy link
Member

agrare commented Nov 29, 2017

@AlonaKaplan can you add a description of what this change is going to do? Are you trying to add a <Template> network option if nothing else is specified?

@AlonaKaplan
Copy link
Contributor Author

@agrare Exactly. If nothing else is specified, which means, keep the template vnics, we be chosen.

@AlonaKaplan
Copy link
Contributor Author

@agrare can this PR be approved/merged?

@agrare
Copy link
Member

agrare commented Dec 8, 2017

@AlonaKaplan can you add a description of what you're trying to do and some background in the PR description instead of a comment?

@AlonaKaplan
Copy link
Contributor Author

@agrare updated the PR description.

@agrare
Copy link
Member

agrare commented Dec 10, 2017

@gmcculloug could we just make the redhat provision dialog default to <Template> instead of having to add this?

@AlonaKaplan
Copy link
Contributor Author

@agrare what is blocking this PR from being merged? @gmcculloug do you have an answer to Adam's questions?

@agrare
Copy link
Member

agrare commented Dec 17, 2017

what is blocking this PR from being merged?

Just that this seems like a weird way to set a default value in a provision dialog. If there isn't a better way to do this then maybe vlan ||= default_vlan where you can set def default_vlan; "<Template>"; end in the provider repo

@gmcculloug
Copy link
Member

@agrare I would hope that setting a default value on the field in the dialog yaml file would work, but would need to be tested since the logic here is trying to set the default based on the source VM might be messing with the normal default logic.

If that does not work I do like the idea of using vlan ||= default_vlan, but to take it one step further the default_vlan method could load the field and and pull the value from there. A little round-about but it would still allow the user to modify the default from the dialog as they would expect.

@miq-bot
Copy link
Member

miq-bot commented Dec 24, 2017

Checked commit AlonaKaplan@f997fd5 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@AlonaKaplan
Copy link
Contributor Author

@agrare @gmcculloug the code was updated per your request.

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

Looks much better! I'll leave this up to @gmcculloug though as I'm not as familiar with the miq_request_workflow

@AlonaKaplan
Copy link
Contributor Author

@gmcculloug can you please review?

@gmcculloug
Copy link
Member

@AlonaKaplan Agreed that this is a much nicer solution. I think you also want to apply the changes to the miq_provision_redhat_dialogs_clone_to_vm.yaml file.

Since the redhat 'provision_workfolw' automatically set the default 'vlan',
there is no need for the 'vlan' field to be required.
@gmcculloug gmcculloug merged commit 30631c2 into ManageIQ:master Jan 10, 2018
@gmcculloug gmcculloug added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 10, 2018
@AlonaKaplan
Copy link
Contributor Author

@miq-bot add_label gaprindashvili/yes

simaishi pushed a commit that referenced this pull request Jan 11, 2018
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit af1170b93c9e174def52c8c9fb7b8f39b2a1a890
Author: Greg McCullough <[email protected]>
Date:   Wed Jan 10 13:32:52 2018 -0500

    Merge pull request #16504 from AlonaKaplan/master
    
    Adding the option to set default vlan
    (cherry picked from commit 30631c2f1a8d9bcd964b8fa85a1870f8fa07993e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1533499

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.

6 participants