Closed #1102.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1102#event-1100660531
nacx approved this pull request.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1102#pullrequestreview-40723452
hi @nacx, per our conversation on IRC I have reverted to using a custom
annotation, but put a
[comment](https://github.com/jclouds/jclouds/pull/1102/files#diff-d646d0465e56af59039c3489721bf61aR29)
on it explaining that it is meant only as a temporary measure to decouple the
new function in
@nacx ok; I'll try to take a look at the tests to see if I can contribute
anything to a fix. I'd be keen to get this merged so I can start using it.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
>I take it you get the error in master too?
Yes. Something to investigate. But I'd rather keep this PR on hold for a while,
just to make sure auth failures don't come (apart from other things) from
signature issues?
--
You are receiving this because you are subscribed to this thread.
Reply to
Ah, so it's something to do with the tests themselves then, not just my
environment being wrong? I take it you get the error in `master` too?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
I don't know why it raises that unauthorized error. I tried with my account
with the same results.
Also tried modifying the test to bind the form4 signer by default, without
success, and creating a specific test in the awsec2 provider that extends the
one in the generic ec2, but with the same
p.s. I have checked the same tests as above in `master` and they fail in the
same way for me, so it is something to do with my environment or AWS account,
not a feature of the PR itself.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view
@geomacy pushed 1 commit.
70860c7 Add comment explaining the @SinceApiVersion override.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
hi @nacx
thanks for the explanation of the wiring.
I've tried to run live tests, but may not be set up for it - the tests have
`@SinceApiVersion` are
```
find . -name '*Api.java' | xargs grep -l SinceApiVersion | sed 's#.*/##' |
while read file ; do find . -name ${file%.java}LiveTest.java;
Good!
Have you been able to run the live tests for the affected APIs?
>By the way how is the @ApiVersion actually injected in places like FormSigner,
>for example? I think the value is being taken from AWSEC2ApiMetadata but I
>couldn't work out how jclouds configures Guice (or whatever) to
Updates made.
I was able to switch to using `@SinceApiVersion`, very pleased with that. The
[key
thing](https://github.com/jclouds/jclouds/pull/1102/commits/3bff59d28c5260e18504c6570ad5cb47117a3d48#diff-7d8365e45e83df7988b1084146439d71R158)
is that I needed to change the override from a
@geomacy pushed 3 commits.
3bff59d Use @SinceApiVersion rather than a custom annotation.
110b188 Use Optional rather than returning null.
74e4245 Apply the version check in FormSignerV2 as well as V4
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
Unit tests may fail because they hardcode the expected version. Live tests will
tell us if the change is safe. Let's run them for the classes that use the
annotation.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@nacx
I can add the annotation at class level.
I originally started using `@SinceApiVersion`, and having the annotation at
both method and class level, but found that various tests broke, because
various APIs are annotated `@SinceApiVersion`.I will take a look at that
again, but
I like the annotation approach and agree that the flexibility needed in ARM,
where they deploy new versions of the APIs quite often (but keeping backward
compatibility) might not be needed here.
A couple questions:
* It would be worth considering the annotation at class level too. Could you
I think we should edit the FormSignerV2 as well
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1102#issuecomment-302394483
andreaturli commented on this pull request.
looks really useful as we discussed on IRC.
Some minor comments, but @geomacy your impl is even cleaner than expected using
the annotation.
Thanks!
> @@ -101,7 +105,25 @@
this.serviceAndRegion = serviceAndRegion;
}
- @Override public
This is a follow-up to
https://github.com/jclouds/jclouds/pull/1091#issuecomment-299202429.
> +1 to merging this but I have been trying this out and I think we will need
> to extend it for practical purposes; if you want to create a VPC and subnet
> and then depl>oy a machine on to it, you
19 matches
Mail list logo
