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

Fix regression in assume role caching #962

Merged
merged 3 commits into from
Jun 22, 2016

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Jun 22, 2016

This pulls in #961 and adds tests for the change.

Right now you get an error:

$ aws ec2 describe-instances --profile some-role
Value cannot be cached, must be JSON serializable: {u'AssumedRole...

This was from a recent addition to botocore that added the HTTP headers into the response metadata returned from client calls.

I agree with the original change from @bashtoni to fix this in the parser. We've had an implicit contract that you can JSON dump response metadata so while we could fix this in the assume role cacher, it's possible customer code could be relying on this property, in which case we would not be able to fix.

cc @kyleknap @JordonPhillips

Sam Bashton and others added 3 commits June 22, 2016 13:26
@jamesls jamesls added the pr/needs-review This PR needs a review from a member of the team. label Jun 22, 2016
@codecov-io
Copy link

Current coverage is 97.31%

Merging #962 into develop will not change coverage

@@            develop       #962   diff @@
==========================================
  Files            43         43          
  Lines          7014       7014          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           6826       6826          
  Misses          188        188          
  Partials          0          0          

Powered by Codecov. Last updated by fac5529...2f151ae

@JordonPhillips
Copy link
Contributor

--    .-""-.
   ) (     )
  (   )   (
     /     )
    (_    _)                     0_,-.__
      (_  )_                     |_.-._/
       (    )                    |_--..\
        (__)                     |__--_/
     |''   ``\                   |
     |        \                  |      /b.
     |         \  ,,,---===?A`\  |  ,==y'
   ___,,,,,---==""\        |M] \ | ;|\ |>
           _   _   \   ___,|H,,---==""""bno,
    o  O  (_) (_)   \ /          _     AWAW/
                     /         _(+)_  dMM/
      \@_,,,,,,---=="   \      \\|//  MW/
--''''"                         ===  d/
                                    //
                                    ,'__________________________
   \    \    \     \               ,/~~~~~~~~~~~~~~~~~~~~~~~~~~~
                         _____    ,'  ~~~   .-""-.~~~~~~  .-""-.
      .-""-.           ///==---   /`-._ ..-'      -.__..-'
            `-.__..-' =====\\\\\\ V/  .---\.
                      ~~~~~~~~~~~~, _',--/_.\  .-""-.
                            .-""-.___` --  \|         -.__..-

@kyleknap
Copy link
Contributor

🚢

@kyleknap kyleknap added pr/ready-to-merge This PR is ready to be merged. and removed pr/needs-review This PR needs a review from a member of the team. labels Jun 22, 2016
@jamesls jamesls merged commit 2f151ae into boto:develop Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready-to-merge This PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants