Skip to content

Conversation

@dnitsch
Copy link
Collaborator

@dnitsch dnitsch commented Sep 27, 2025


name:

Describe the PR
added more sections to the config file

fix: add more tests

rework how the config file is passed in

perform merges from config.specific sections to the global config

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

+semver: feature
+semver: FEATURE

rework how the config file is passed in

perform merges from config.specific sections to the global config

+semver: feature
+semver: FEATURE
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds the ability to specify global and specific parameters through a config INI file, enhancing the configuration management system.

Key changes include:

  • Introduces configuration file parsing with global [config] sections and specific [config.specific_section] sections
  • Adds support for merging configuration from files with command-line flags
  • Refactors method signatures to pass configuration data explicitly rather than loading it internally

Reviewed Changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/web/web.go Adds method to set custom browser executable path
internal/credentialexchange/secret.go Refactors ClearAll method to accept INI file parameter and moves method positioning
internal/credentialexchange/helper.go Adds LoadCliConfig function for merging global and specific configuration sections
internal/credentialexchange/config.go Adds INI tags to struct fields for configuration mapping
cmd/saml.go Major refactoring to support configuration file loading and flag merging
cmd/awscliauth.go Updates root command flags structure and adds config file location parameter
cmd/clear.go Updates to use new ClearAll method signature
Comments suppressed due to low confidence (1)

internal/credentialexchange/helper_test.go:1

  • All t.Error() calls should include descriptive error messages explaining what was expected vs actual values to aid in debugging test failures.
package credentialexchange_test

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

for name, tt := range ttests {
t.Run(name, func(t *testing.T) {
tmpDir, _ := os.MkdirTemp(os.TempDir(), "saml-cred-test")
tmpDir, _ := os.MkdirTemp(os.TempDir(), "saml-cred-test-*")
Copy link

Copilot AI Sep 27, 2025

Choose a reason for hiding this comment

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

Error from os.MkdirTemp should be checked and handled. The underscore assignment ignores potential errors that could cause test failures.

Suggested change
tmpDir, _ := os.MkdirTemp(os.TempDir(), "saml-cred-test-*")
tmpDir, err := os.MkdirTemp(os.TempDir(), "saml-cred-test-*")
if err != nil {
t.Fatal(err)
}

Copilot uses AI. Check for mistakes.
@sonarqubecloud
Copy link

@dnitsch dnitsch merged commit c08afec into master Sep 29, 2025
4 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.

2 participants