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 #2015 - Update topsites.py for IAM user credential #2021

Merged
merged 1 commit into from
Jan 19, 2018

Conversation

MDTsai
Copy link
Contributor

@MDTsai MDTsai commented Jan 12, 2018

Use HTTP GET and different signature then use root credential. @karlcow could you help to review this?

@karlcow
Copy link
Member

karlcow commented Jan 12, 2018

@MDTsai yes I can help. Do you need to get it done by today or by monday?

@MDTsai
Copy link
Contributor Author

MDTsai commented Jan 12, 2018

@karlcow by end of January, old credential fail day, thanks !

@karlcow karlcow self-requested a review January 12, 2018 11:58
@miketaylr
Copy link
Member

(drive by comment, just to see why tests failed):

$ pep8 --ignore=E402 webcompat/ tests/ config/secrets.py.example
tests/test_topsites.py:21:80: E501 line too long (221 > 79 characters)
tests/test_topsites.py:24:1: E302 expected 2 blank lines, found 1

Use HTTP GET and different signature then use root credential
Fix unittest cases
@MDTsai
Copy link
Contributor Author

MDTsai commented Jan 16, 2018

Just fix the PEP8 fail

Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

@MDTsai Awesome work as usual. ❤️ Thanks.

All the comments and requests for changes are syntactic.

  • So I would replace \n by string structure (though to be tested, specifically for the signing process.)
  • The code needs to be merged with the latest unit tests structure introduced in Fixes #1986 - Start migration to Intern 4 #1987 But this is smooth I have done it locally. :)
  • Remove import base64 in the topsites.py
  • There are a couple of long lines which needs to be reformatted and/or have # noqa or # nopep8 keywords

query=canonical_query,
headers=canonical_headers,
signed_headers=ATS_SIGNED_HEADERS,
payload_hash=payload_hash)

This comment was marked as abuse.

return sign_key


def get_sha256_hex(data):

This comment was marked as abuse.

return urlencode(query_params)


def gen_sign(data):
def get_sign_key(key, datestamp, region_name, service_name):
"""AWS sign key from key, datestamp, region and service"""

This comment was marked as abuse.

@karlcow
Copy link
Member

karlcow commented Jan 18, 2018

No worries @MDTsai
It's very good work. As usual! 🏆
I can probably add the changes during the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants