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

fix: quiet flag to stop credential process errors #160

Merged

Conversation

joshrivers
Copy link
Contributor

Resolves #159

  • main.initializeLogger() now installs a no-op logger as the global logger if a "quiet" flag has been passed to the CLI argument parser
    • Added coverage for initializeLogger to establish its behavior with different flags or defaults
  • Added -q, --quiet, and --non-interactive flags as options on the assume subcommand. These could be moved to a higher scope later if useful, but they currently only appear to be relevant to this subcommand
  • Modified the credential process template to add the -q flag on profile creation
  • Updated README with current help output
  • Changed references to ioutil.ReadFile to os.ReadFile to resolve deprecation notes
  • Partially resolved a unit test race condition which was making GitHub Actions builds fail intermittently
    • Purged build caches, since any successful runs would prevent subsequent failures
    • Added pre- and post- removal of the lock-file to allow test runs to complete without stopping due to the lock
    • Ensured the defer functions were established before test operations which could crash the process

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:

  • The executable name provided in the credential process template can include spaces from the file system path. This can cause failures if the go-aws-sso executable is not in a "normal" location. I'm not 100% certain that just quoting the fully qualified executable path in that file works cross-platform. Go does not have a batteries-included version of the Python shlex.quote function and I didn't want to add a dependency for a different issue than the one I was working on.

@theurichde theurichde self-requested a review December 27, 2023 08:44
Copy link
Owner

@theurichde theurichde left a comment

Choose a reason for hiding this comment

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

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?

README.md Outdated Show resolved Hide resolved
cmd/go-aws-sso/main.go Outdated Show resolved Hide resolved
@theurichde
Copy link
Owner

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
Copy link
Owner

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.

@theurichde theurichde self-requested a review January 3, 2024 19:38
@theurichde theurichde linked an issue Jan 3, 2024 that may be closed by this pull request
@joshrivers
Copy link
Contributor Author

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.

joshrivers and others added 3 commits January 4, 2024 22:33
The quiet flag and alias are sufficient for the need and the redundant non-interactive flag can be left out

Co-authored-by: Tim Heurich <[email protected]>
@theurichde theurichde force-pushed the fix/quiet-flag-assume-command branch from 4e984c4 to 190575e Compare January 4, 2024 21:33
@theurichde theurichde merged commit 07ecbd0 into theurichde:main Jan 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra data error when used as Credential Provider
2 participants