JCLOUDS-543: hack to not barf on old images that are no longer listed in the api #58
Conversation
|
jclouds » jclouds-labs #982 SUCCESS |
|
jclouds-labs-pull-requests #141 SUCCESS |
|
Thanks for the contribution @cobbzilla! Out of curiosity, what happens if you just leave the image (and all image derived fields) to It's nice to have DO users and to see use cases like this! Your long-living droplets will help us umderstand some of their api internals :) |
Would an |
|
That would be cleaner but require to change the portable model (affecting all providers). For the moment I'd see what happens when leaving them |
Agreed...if this is a corner case, the minimal possible change sounds sensible (how about a "Null image object", then, i.e. something like |
|
The point is that the model only exposes the imageId, which is a String, and the OperatingSystem, both already marked as @nullable in the NodeMetadata interface; the entire image object itself is not exposed |
Oops...lazy me ;-) In that case, |
|
And also lazy me! I didn't express it properly in my first comment :) It's always good to discuss these kind of details! |
|
@nacx I'm not sure I understand your question -- if I let the "image" variable be null, the very next line ( |
|
I meant to not assign the imageId and operatingSystem if the image is null. Could you try that? |
|
@nacx I see what you mean -- yes I can try that. I'll report back after I've given it a whirl. |
|
yup, that change seems to have worked fine -- the integration test I have that uses this code still passes A-OK. |
|
jclouds-labs-pull-requests #142 SUCCESS |
|
jclouds » jclouds-labs #984 SUCCESS |
| @@ -91,8 +91,10 @@ public boolean apply(Location location) { | |||
| })); | |||
|
|
|||
| Image image = images.get().get(String.valueOf(input.getImageId())); | |||
| builder.imageId(image.getId()); | |||
| builder.operatingSystem(image.getOperatingSystem()); | |||
| if (image != null) { | |||
There was a problem hiding this comment.
Please add a comment describing under which circumstances this may occur?
@nacx: I guess we should also have a JIRA issue and a unit test for this?
I've just opened JCLOUDS-543 to track this. There is no unit test class for the |
demobox
changed the title
Thanks, @nacx! |
|
I added the comment. Re adding a test, I have to admit I'm pretty swamped and I may take a long while getting to it. Time permitting, I will try. |
|
jclouds-labs-pull-requests #143 SUCCESS |
|
jclouds » jclouds-labs #985 SUCCESS |
|
Thanks @cobbzilla. I'm working on a PR to add tests for the existing functionality in those transformation functions (let's fix our technical debt!) and hope to open it tomorrow. If it is ok to you, you can wait until then, and just add your test to the ones I'll create. |
|
thanks @nacx -- I would much prefer adding one test to an existing test suite that clearly illustrates the patterns/conventions you'd like to see followed, versus stumbling around in the dark :) If you do end up creating some tests, could you post to this thread and point me to the appropriate place where you'd like me to add a test for this PR? |
|
@nacx @cobbzilla Ping on this? Just wondering where we're currently at here..? |
|
I plan to merge the checkstyle PR tomorrow and then open a new one adding the test classes. So in a day or two everything should be in place to make it trivial to add the missing tests! |
|
Thanks for thw ping @demobox! I started this but completely forgot! :) |
|
@demobox I will patiently wait for some kind of exemplary test framework that's easy to understand so I can quickly add a test to it. It'd be nice to get this PR merged in, but I'm not in a huge rush. |
Ah, great. Thanks, @nacx! |
|
@demobox @cobbzilla I've just opened #62 adding the missing tests. |
|
This is also affecting the Jenkins plugin: As commented in the issue there, now that thay've added support for slugs everywhere I think the right fix would be to use them everywhere instead of the ids. I'll try to have a PR for that soon if you don't beat me! :) |
I'm not in the race, but perhaps @cobbzilla is ;-) |
|
I've just opened #63 adding support for using the "slug" values as IDs to properly fix the entire thing, now that we know more a bit more about the DO api. It was not as straightforward as I had initially supposed (now methods have been added to the api), but I think it will make the provider easier to use. If that PR buids without issues (I've executed the live tests locally and all pass), I think we'd better merge it and close this one. Are you ok with this @cobbzilla? Many thanks for the investigation and the pointers! That has definitely been the key point to these improvements and helped a lot identifying the Jenkins issue! |
|
Sure -- I'm fine with merging it. The only potential problem would be if some code relied on the builder having a non-null imageId or operatingSystem, in which case we've just pushed the NPE to somewhere else. But the alternative is to bomb out on a lot of stuff that would otherwise work just fine with a null imageId/operatingSystem. ceteris paribus, I think this fix is the better way. |
preface -- I'm sure there is probably a better way to do this and I'm open to suggestions on changes.
the problem: some of the droplets in my account are long-running, and have image_ids that are no longer returned from DO's /images api call. So, when I'm trying to create a new droplet, it fails here with a NPE while iterating over the existing droplets (trying to find ones that might match the group name).