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

Fixes request path for ex_set_image_labels() #1485

Closed
wants to merge 2 commits into from

Conversation

petersen-poul
Copy link
Contributor

Fixes request path for ex_set_image_labels()

Description

A simple fix - the request in ex_set_image_labels was missing the "/images/" path.

I made this commit against 2.8.x because that's where I needed it, but it's also missing in trunk.

Status

Replace this: describe the PR status. Examples:

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

@Kami
Copy link
Member

Kami commented Sep 3, 2020

Thanks for the contribution.

The change looks good to me, but it looks like existing tests are failing (can you please look into that?). In addition to that, it would be also good to add a test case for this fix.

@@ -2080,7 +2080,7 @@ def ex_set_image_labels(self, image, labels):
raise ValueError("Must specify a valid libcloud image object.")
current_fp = image.extra['labelFingerprint']
body = {'labels': labels, 'labelFingerprint': current_fp}
request = '/global/%s/setLabels' % (image.name)
request = '/global/images/%s/setLabels' % (image.name)
Copy link
Member

Choose a reason for hiding this comment

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

@asfgit asfgit closed this in 5691fd0 Sep 3, 2020
@Kami
Copy link
Member

Kami commented Sep 3, 2020

I fixed the failing tests, added an assertion (d968345) and merged changes into trunk.

@petersen-poul I had some troubles merging changes directly into trunk so I needed to manually cherry pick the commits.

It looks like your branch is not based on trunk, but on top of 2.8.x - I assume you are using v2.8.x release series which still supports Python 2?

At this point, I only merged change into trunk (those changes will be included in the next v3.x.x release), but perhaps if some other bug fixes accumulate, we can make another v2.8.x release.

@petersen-poul
Copy link
Contributor Author

Whoops - my apologies for not noticing your comments earlier. Yes, I'm stuck in v2.8.3 for now due to Python 2.7. Yes, the changes for trunk should be virtually the same. If I find anything else in my migration efforts, I'll update them as well.

d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 15, 2021
d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 19, 2021
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.

None yet

2 participants