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 vspk python module for Nuage #292

Merged
merged 1 commit into from
Oct 12, 2018
Merged

Conversation

simaishi
Copy link
Contributor

@simaishi simaishi commented Oct 9, 2018

https://bugzilla.redhat.com/show_bug.cgi?id=1630801

Bambou (which is also listed in the BZ) is a dependency of vspk.

@simaishi simaishi changed the title Add vspk python module, needed for Nuage Add vspk python module for Nuage Oct 9, 2018
@carbonin
Copy link
Member

Is this going to change our upgrade procedure? Will we need to start upgrading the pip packages in some way? Also, are there any version constraints? I know the BZ says the latest version, but that seems awfully optimistic... Are they sure that no new version will break their integration?

@carbonin
Copy link
Member

cc @jrafanie @Fryguy thoughts on this?

@jrafanie
Copy link
Member

Is this going to change our upgrade procedure? Will we need to start upgrading the pip packages in some way? Also, are there any version constraints? I know the BZ says the latest version, but that seems awfully optimistic... Are they sure that no new version will break their integration?

These are all good questions. Can we get @Loicavenel to comment here? While, we can certainly install the latest pip and vspk, it's going to the "latest" packaged versions, which is usually super old. It would be great to know what minimum versions we need and if there are any consumers of it to help verify it still works on upgrades. "For nuage" doesn't really tell us much.

@simaishi
Copy link
Contributor Author

vspk is coming straight from pypi, so it will be the latest available at the time of appliance build.

I think the future plan is to use requirements.txt, and we'll be able to specify version requirement there. So the concern for now will really be if we might take packages that's newer than what we want to use.

cc @miha-plesko

@jrafanie
Copy link
Member

vspk is coming straight from pypi, so it will be the latest available at the time of appliance build.

I think the future plan is to use requirements.txt, and we'll be able to specify version requirement there. So the concern for now will really be if we might take packages that's newer than what we want to use.

Right, my bad. Only python-pip will be from a package. The means each build of manageiq may get different versions of vspk depending on how frequently it's changed.

@simaishi
Copy link
Contributor Author

The means each build of manageiq may get different versions of vspk depending on how frequently it's changed.

I think that itself isn't a problem as that's how gem (as well as rpm) works too. The only difference here is that we can't prevent breaking version coming to the build as we don't have requirements.txt yet..

@miha-plesko, I think your input will be needed here... if you'd like version constraints to be set now, we can do that, please let me know what it should be. Or we can leave it open and assume breaking change in vspk (if any) will come with code update...

@miha-plesko
Copy link

@simaishi lets lock it to vspk==5.3.2, please. We wrote "latest" to the BZ long time ago when we didn't know when the integration will be complete. Well, it is complete enough now that we can lock the version.

@simaishi
Copy link
Contributor Author

Updated to lock to 5.3.2

@miq-bot
Copy link
Member

miq-bot commented Oct 12, 2018

Checked commit simaishi@fa8eac8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM to me for now. I don't know how we track when this version needs to be changed but we can tackle that when it needs to change. What do you think @carbonin?

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

Definitely more comfortable with this now that the version is locked. 👍

@carbonin carbonin merged commit 146dae9 into ManageIQ:master Oct 12, 2018
@carbonin carbonin self-assigned this Oct 12, 2018
@carbonin carbonin added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 12, 2018
@simaishi simaishi deleted the add_python_vspk branch October 12, 2018 21:12
simaishi pushed a commit that referenced this pull request Oct 15, 2018
@simaishi
Copy link
Contributor Author

Hammer backport details:

$ git log -1
commit ddec56e5c4b759aa210ebffa7db833ab01e65412
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Fri Oct 12 17:10:35 2018 -0400

    Merge pull request #292 from simaishi/add_python_vspk
    
    Add vspk python module for Nuage
    
    (cherry picked from commit 146dae92db0153117f36bdb771e862eec2976e7d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1630801

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