theurichde / go-aws-sso

Makes dealing with AWS SSO Logins an ease
MIT License
111 stars 15 forks source link

fix: quiet flag to stop credential process errors #160

Closed joshrivers closed 9 months ago

joshrivers commented 9 months ago

Resolves #159

Discussion: the root problem here is the frequent use of zap.Fatal() through the application code. This causes the test execution process to immediately terminate without collecting failure logging when any application error occurs. Since the zap logger is not initialized for test runs, it is difficult to see where the problem is happening without debugging. Debugging is hard since it worked 100% of the time on my machine, only failing in the GitLab Actions. I wasn't able to determine if the failure was due to the main and assume tests having an interaction because of concurrency, or due to interactions between subsequent runs in the Actions environment with restored caches. I suspect the former, but it got late. I was thinking that the same behavior, but with better failure notification in the tests might be achieved by using zap.Panic() rather than zap.Fatal(), but I haven't thought through the implications. Providing a wrapper interface/factory for zap that could be overridden with a mock in the tests would also solve the same problems. The temp file location might also have a testing override to ensure that test cases are isolated from each other.

Not fixed:

theurichde commented 9 months ago

Discussion: the root problem here is the frequent use of zap.Fatal() through the application code.

I will take that point with me and create an issue out of it to have a look and work on it. Thanks for the detailed input! Edgy edge cases sometimes bite back.

theurichde commented 9 months ago

Not fixed: The executable name provided in the credential process template can include spaces from the file system path.

Interesting point! I will also create an issue for that, to look into and play around with it.

joshrivers commented 9 months ago

Sorry for keeping you waiting so long. Holiday season is finally over, time for some review 🔍 😀

First of all: Thanks for your PR, your contribution and your input! I really appreciate that!

Regarding the non-interactive alias - I would simply drop it, as it adds no benefit imo; --quiet or -q are totally fine.

What do you think?

I agree. Keeping it simple is better.