Readme: Update KMSMasterKeyProvider credentials configuration for bet…

[nraval1729]

…ter understanding

Issue #, if available:
#88

Description of changes:
Differentiate between the two ways to configure credentials for the KMSMasterKeyProvider and reword confusing bits.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

• Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

[mattsb42-aws]

Thanks for the contribution!

In the changes staged in keyring (that will be landing in master soon #249 ) we've significantly pared down the readme, deferring content to examples, API docs, and the developer guide. With that in mind, let's move this into the KMSMasterKeyProvider docstring.

On a formatting note, we use semantic newlines[1] for our documentation and have a max line length of 120, enforced by our CI checks using flake8 and doc8.

[1] https://rhodesmill.org/brandon/2012/one-sentence-per-line/

[nraval1729]

That article was enlightening! I'm going to introduce it in my engineering team as well. Thanks a bunch for sharing 😄

I've moved the KMSMasterKeyProvider related README code into its docstring. Let me know what you think @mattsb42-aws

Also, I notice 3 failing checks. I dug around and also noticed them in one of your PRs (#255). Is this a known issue? I just wanted to ask before investing time to fix/investigate it.

[mattsb42-aws]

That article was enlightening! I'm going to introduce it in my engineering team as well. Thanks a bunch for sharing 😄

Awesome! :) attrs introduced them to me and I've come to love the effect they can have in simplifying edits to documentation source and organizing thoughts within that source.

Also, I notice 3 failing checks. I dug around and also noticed them in one of your PRs (#255). Is this a known issue? I just wanted to ask before investing time to fix/investigate it.

Yes, those are expected for now. When I moved a bunch of our CI checks from Travis into GitHub Actions, I enabled some checks that were previously disabled because they were failing. I'm leaving them enabled to remind me to fix them, but you'll notice that they are not required.

[nraval1729]

Awesome! :) attrs introduced them to me and I've come to love the effect they can have in simplifying edits to documentation source and organizing thoughts within that source.

Sweet. I've not heard of that package. I'll check it out!

Yes, those are expected for now. When I moved a bunch of our CI checks from Travis into GitHub Actions, I enabled some checks that were previously disabled because they were failing. I'm leaving them enabled to remind me to fix them, but you'll notice that they are not required.

Understood.

Do the changes look good or should we trim down some stuff from the docstring?

As a side note, I was wondering why the team decided to move the documentation into classes and by way of examples. I've seen documentation on either side - defined in detail in a Readme and delegated into the code/examples itself. I have personally felt it easier to follow documentation that was laid out in a Readme because of the natural instinct of going through it when trying to work with a project. I personally prefer to dive into the code when I either want to understand its implementation or just get feel for it once I'm already comfortable with the project. Just wanted to know the thought process behind the change :)

[mattsb42-aws]

Do the changes look good or should we trim down some stuff from the docstring?

I updated the PR branch with some edits. Let me know if anything doesn't make sense or needs clarification.

As a side note, I was wondering why the team decided to move the documentation into classes and by way of examples.

The biggest reason is to reduce the number of places where we have duplicate content and to make sure that any examples that we show are actually tested in our CI.

The readme had previously contained three main sections: how to install, concepts, and examples. The concepts section now links to the developer guide[1] because that is our source of truth for explaining those concepts and to avoid the inevitable drift if we were to attempt to maintain duplicate descriptions across each of our implementations[2]. The examples now link to the full examples rather than duplicating that content in the readme, where among other issues they're hard to test. Related to this, we've also been adding a lot of new examples to try to better demonstrate common patterns.

In general the place that I recommend people start looking through documentation for this project is the developer guide[1] for a higher-level understanding of the tool and then the API docs[3] for specifics on this implementation. The API docs are where these docstrings are rendered[4]. You can see how they render locally by running tox -e docs,serve-docs and navigating to localhost:8000 in a browser.

[1] https://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/introduction.html
[2] https://github.com/aws/crypto-tools#aws-encryption-sdk
[3] https://aws-encryption-sdk-python.readthedocs.io
[4] https://aws-encryption-sdk-python.readthedocs.io/en/latest/generated/aws_encryption_sdk.key_providers.kms.html#aws_encryption_sdk.key_providers.kms.KMSMasterKeyProvider

[nraval1729]

Should we move the credentials related information towards the top of the docstring (above line #85 (To encrypt data, you must configure ...) or is it okay either way? I am asking because conventionally information about how to set credentials appears before other information.

[mattsb42-aws]

I put the CMK configuration first because I expect more users to need to know the affect of CMK configuration than will need to know how to configure credentials. In the happy-path of "I'm running in AWS in an environment where the credentials magic in (EC2, ECS, Lambda, etc)", you usually don't need to think about the credentials at all; the default behavior will Just Work.

[nraval1729]

Good point. Makes sense 👍

[nraval1729]

Thanks for the thoughtful response.

Your points about preventing the drift between the source of truth and the Readme, and ensuring that whatever code samples we give out are CI-test-suite-certified really hit the nail on the head.

Everything looks good to me other than a minor point that I've commented on. Other than that feel free to merge it 😄