Bugfix/issue 325 - Githubissues

glasswalk3r commented 5 years ago

Changed the way Awspec::Helper::Finder instantiates clients so first it checks for exception related to configurations done wrong.
Awspec::Helper::Finder::Alb now checks for Aws::ElasticLoadBalancingV2::Errors::LoadBalancerNotFound exception instead of generic ones.
Added check_instance to be executed at each method from Awspec::Helper::Finder::Alb so a proper exception is generated if there is no such load balancer.

glasswalk3r commented 5 years ago

Ops... @k1LoW , looks like we need to design some way to the tests running on Travis define a AWS region, or we use something that disables this tests to execute over there... otherwise, they will always fail.

k1LoW commented 5 years ago

awspec should not check ENV, too. I think this is beyond the awspec's responsibilities.

awspec only uses Aws. awssecrets sets credentials.

glasswalk3r commented 5 years ago

awspec should not check ENV, too. I think this is beyond the awspec's responsibilities.

awspec only uses Aws. awssecrets sets credentials.

Got that, but since I removed any dependency (awssecrets), now the Travis CI tests will always be broken because this PR forces the configuration of the AWS Client at the very beginning, which will always fail because the Specs on awspec are using stubs, not real configuration.

A possibility is to add an environment variable (for awspec internal usage) that disables this specific test execution if that variable is defined.

k1LoW commented 5 years ago

this PR forces the configuration of the AWS Client at the very beginning

https://github.com/k1LoW/awspec/pull/458/files#diff-f46513d75a662c430cff22e30e77570cR157

I do not feel the need for very beginning configuration of the AWS Client. Because, I think this is beyond the awspec's responsibilities too.

glasswalk3r commented 5 years ago

this PR forces the configuration of the AWS Client at the very beginning

https://github.com/k1LoW/awspec/pull/458/files#diff-f46513d75a662c430cff22e30e77570cR157

I do not feel the need for very beginning configuration of the AWS Client. Because, I think this is beyond the awspec's responsibilities too.

I understand that, but there is a trade-off here: AWS configuration is not responsibility of awspec, but misconfiguration of it will cause a delayed exception, later in the specs execution, with a confusing message that will leave the user wondering if there is an error in the defined specs, awspec, AWS SDK or AWS itself.

Since we are not checking for anything regarding how the configuration was done (possibly repeating validations already provided by the SDK), forcing the client usage before running anything not only will provide a better error message, but also avoid wasting execution time for specs that will surely fail anyway. Future updates on the SDK (and how the configuration is made) will not break up awspec since we don't care how the configuration is done, just if it is working as expected.

What we would need is to define a mechanism to disable this self-test when running on Travis CI, since the stubs will not provide a client configuration by default.

majormoses commented 5 years ago

awspec should not check ENV, too. I think this is beyond the awspec's responsibilities.

awspec only uses Aws. awssecrets sets credentials.

I disagree, it seems weird to have to call some kind of script to init credentials and then call awspec. While I agree it makes it safer long term in case aws changes their auth api but this has not happened in a very long time and I imagine it will break too many tools that they just wont bother.

k1LoW commented 5 years ago

There are a number of AWS credential configuration patterns. There are also many patterns for using credentials. I think it's difficult to support it all with awspec. I want to simplify the awspec's responsibilities to keep the maintenance going.

glasswalk3r commented 5 years ago

@k1LoW , I updated my PR to make it pass on Travis CI by adding a environment variable that disable the AWS client test. I don't think so it will get more simple than that, no assumptions are made regarding how the authentication was done, just executing the client with anything the AWS SDK will pull off from the environment.

glasswalk3r commented 4 years ago

@k1LoW , any chance to have this patch applied?

glasswalk3r commented 4 years ago

Thanks @majormoses !

k1LoW commented 4 years ago

@glasswalk3r

First, thank you for committing to awspec !

This pull request is trying to solve multiple issues. I agree No.3.

.gitignore
- Contains unnecessary file information.
Credential check
- https://github.com/k1LoW/awspec/pull/458#issuecomment-514608746
- If you want to check credentials, please send Pull Request to awsecret instead of awspec.
alb check_existence
- Thank you !

If possible, solve one problem with one PR 🙏 because

I can not merge when only one of them has agreed.
awspec use Pull Request title as changelog.

Best regards.

glasswalk3r commented 4 years ago

Alright @k1LoW , I broke down this PR in two. For the ALB, please check #503 . For awsecrets, I started working on it, but there are pull requests 26 and 28 that you want to check first before I'm able to actually create the PR #325 (almost done basically).

glasswalk3r commented 4 years ago

@k1LoW , the respective PR's for awsecrets are already merged and in place. I just fixed a merge conflict and also fixed, could you please check if this is ready for merge too? Thanks!

k1LoW commented 3 years ago

@glasswalk3r

Could you fix Number one on the list?

glasswalk3r commented 3 years ago

@k1LoW , any chance to have this patch applied?

Gladly! But I had to fork again from awspec because of conflicts when trying to fetch from upstream, and my original branch is lost (I even tried to recreate with if the same name, but Github wasn't fooled).

So, I just copied the changes I made from there to my new fork/branch and will create the PR.

For now, I close this one so it can be done.

k1LoW / awspec

Bugfix/issue 325 #458