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

Fixes #2990 and #2518. Avoid AttributeError when awscli is invoked fr… #2991

Closed
wants to merge 1 commit into from

Conversation

emurphy
Copy link

@emurphy emurphy commented Nov 28, 2017

…om python3 daemon.

@JordonPhillips
Copy link
Member

I don't think simply making it None would be the best way to handle this. Any time we try to access it we would end up rasing an error like 'NoneType' object has no attribute 'write' which isn't very helpful. I would prefer we add in a function get_binary_stdin with raises an exception with a descriptive method if stdin isn't available. That would delay the exception while also providing a helpful error message.

@emurphy
Copy link
Author

emurphy commented Dec 7, 2017

Making the binary_stdin variable None is consistent with sys.stdin being None and is far better than triggering an error. You are welcome to implement your preferred get_binary_stdin function. In the meantime, please merge #2991 so that the aws-cli works for this use case.

Copy link
Member

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

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

A NoneType exception is not a good experience for a user of a cli tool. The fact that sys.stdin itself will be None just means that we should also do something like this for normal stdin.

@emurphy
Copy link
Author

emurphy commented Dec 11, 2017

There may be a misunderstanding here. First, one thing to clarify is that I represent a customer of AWS. Our company pays hundreds of thousands of dollars for the privilege to use your services. It is my understanding that you are a paid employee or contractor of AWS.

Like other paying customers, we have encountered an error in the aws-cli software. We have helpfully, at our own expense, provided you with a minimal, no-risk solution. Let us analyze the line of code to ensure there is no misunderstanding there…

Before merging this PR:

binary_stdin = sys.stdin.buffer

After merging this PR:

binary_stdin = sys.stdin.buffer if sys.stdin is not None else None

This is simple, a trivial fix.
Two input conditions are worth considering:

  1. sys.stdin is None
  2. sys.stdin is not None

In case 1:
In the Before case the program errors. aws-cli breaks, fails to meet a minimum requirement.
In the After case the program continues, with the variable binary_stdin set to None. Not a problem, meets the minimum need.

In case 2, both Before and After have the same result. binary_stdin is safely set to sys.stdin.buffer. All is well.

I could challenge your statement that this fix is not “the best”, but I believe it is irrelevant. In the before case the software breaks; we are not getting from you what we pay for. In the after case your software works, we get what we pay for.

There are no drawbacks; there are no excuses for your inaction.
Please, merge it now.

I will not comment further. I will not change this PR. Our next step, if this issue is not addressed, will be to mention it to our AWS account manager and open a business support case.

Please, as your humble customer, I beg you… merge it now.

@diehlaws diehlaws added feature-request A feature should be added or improved. and removed enhancement labels Jan 4, 2019
@justnance justnance added enhancement and removed feature-request A feature should be added or improved. labels Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants