Bugfix/issue 325 #458

glasswalk3r · 2019-03-05T23:51:41Z

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.

Changed the way Awspec::Helper::Finder instantiates clients so first it checks for exception related to misconfigurations. 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.

k1LoW · 2019-07-06T22:13:46Z

lib/awspec/helper/finder.rb

@@ -148,6 +148,16 @@ module Finder
      http_proxy: ENV['http_proxy'] || ENV['https_proxy'] || nil
    }

+    # define_method below will "hide" any exception that comes from bad
+    # setup of AWS client, so let's try first to create a instance
+    if File.exist?(ENV['HOME'] + '/.aws') && File.directory?(ENV['HOME'] + '/.aws')


awspec should not check .aws. I think this is beyond the awspec's responsibilities

Just removed the .aws verification... now awspec will try to create a instance of Aws::EC2::Client in any setup situation (.aws or environment variables).

glasswalk3r · 2019-07-15T14:17:26Z

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 · 2019-07-15T16:02:47Z

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

awspec only uses Aws. awssecrets sets credentials.

glasswalk3r · 2019-07-15T18:35:29Z

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 · 2019-07-21T03:12:34Z

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 · 2019-07-22T15:03:43Z

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 · 2019-07-23T02:17:41Z

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 · 2019-07-24T12:21:26Z

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 · 2019-07-28T00:54:00Z

@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 · 2019-11-16T22:11:08Z

@k1LoW , any chance to have this patch applied?

lib/awspec/helper/finder.rb

Co-Authored-By: Ben Abrams <[email protected]>

glasswalk3r · 2019-11-17T00:04:39Z

Thanks @majormoses !

k1LoW · 2019-11-17T09:49:43Z

@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
- Bugfix/issue 325 #458 (comment)
- 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 · 2020-02-03T01:48:54Z

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 · 2020-07-16T03:44:54Z

@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 · 2021-06-28T21:28:05Z

@glasswalk3r

Could you fix Number one on the list?

glasswalk3r · 2021-07-03T14:39:05Z

@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.

Alceu Rodrigues de Freitas Junior added 2 commits March 5, 2019 20:37

Check for configuration before creating client

045550b

glasswalk3r mentioned this pull request Mar 6, 2019

Cryptic exception when region is not set #325

Closed

k1LoW reviewed Jul 6, 2019

View reviewed changes

Alceu Rodrigues de Freitas Junior added 3 commits July 14, 2019 10:18

Merge branch 'master' into bugfix/issue_325

58e0e70

Added gitignore

f4449af

Removed checking for .aws

79a8b6e

Alceu Rodrigues de Freitas Junior added 3 commits July 27, 2019 20:55

Merge branch 'master' into bugfix/issue_325

b9a8ad0

Fixed Travis CI

70706df

Added useful comments

141bdc0

majormoses reviewed Nov 16, 2019

View reviewed changes

lib/awspec/helper/finder.rb Outdated Show resolved Hide resolved

Update lib/awspec/helper/finder.rb

c63fd4d

Co-Authored-By: Ben Abrams <[email protected]>

glasswalk3r added 2 commits July 15, 2020 23:41

PR 458

2d74af4

Merge branch 'master' into bugfix/issue_325

cb2394e

glasswalk3r closed this Jul 3, 2021

glasswalk3r mentioned this pull request Jul 3, 2021

Bugfix/issue 325 #537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/issue 325 #458

Bugfix/issue 325 #458

glasswalk3r commented Mar 5, 2019

k1LoW Jul 6, 2019

glasswalk3r Jul 14, 2019

glasswalk3r commented Jul 15, 2019

k1LoW commented Jul 15, 2019

glasswalk3r commented Jul 15, 2019 •

edited

Loading

k1LoW commented Jul 21, 2019

glasswalk3r commented Jul 22, 2019

majormoses commented Jul 23, 2019 •

edited

Loading

k1LoW commented Jul 24, 2019

glasswalk3r commented Jul 28, 2019

glasswalk3r commented Nov 16, 2019

glasswalk3r commented Nov 17, 2019

k1LoW commented Nov 17, 2019

glasswalk3r commented Feb 3, 2020

glasswalk3r commented Jul 16, 2020

k1LoW commented Jun 28, 2021

glasswalk3r commented Jul 3, 2021

Bugfix/issue 325 #458

Bugfix/issue 325 #458

Conversation

glasswalk3r commented Mar 5, 2019

k1LoW Jul 6, 2019

Choose a reason for hiding this comment

glasswalk3r Jul 14, 2019

Choose a reason for hiding this comment

glasswalk3r commented Jul 15, 2019

k1LoW commented Jul 15, 2019

glasswalk3r commented Jul 15, 2019 • edited Loading

k1LoW commented Jul 21, 2019

glasswalk3r commented Jul 22, 2019

majormoses commented Jul 23, 2019 • edited Loading

k1LoW commented Jul 24, 2019

glasswalk3r commented Jul 28, 2019

glasswalk3r commented Nov 16, 2019

glasswalk3r commented Nov 17, 2019

k1LoW commented Nov 17, 2019

glasswalk3r commented Feb 3, 2020

glasswalk3r commented Jul 16, 2020

k1LoW commented Jun 28, 2021

glasswalk3r commented Jul 3, 2021

glasswalk3r commented Jul 15, 2019 •

edited

Loading

majormoses commented Jul 23, 2019 •

edited

Loading