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

use the global default socket path for spire-agent run #1738

Merged
merged 3 commits into from
Jul 24, 2020

Conversation

joewilliams
Copy link

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
Default socket path for spire-agent run.

Description of change
Make the default spire-agent default socket path more obvious by using the global default value.

Which issue this PR fixes

@joewilliams joewilliams changed the title use the global default socket path use the global default socket path for spire-agent run Jul 22, 2020
@azdagron
Copy link
Member

Code change is looking good!

There is a small risk of breaking backwards compatibility here for folks who don't define the socket_path in their agent configuration who were relying on the strange ./spire_api default. Considering all of the configurations defined by tutorials and examples do define socket_path, it doesn't seem too likely that someone would be reliant on this, but I thought I'd call out the risk.

Thoughts @evan2645 , @amartinezfayo , @APTy , @mcpherrinm? Any concern taking this potentially breaking change? What is your evaluation of the risk?

@joewilliams
Copy link
Author

Not sure why this test is failing while the others are passing:

golangci/golangci-lint info installed /home/travis/gopath/src/github.com/spiffe/spire/.build/linux-x86_64/golangci_lint/v1.27.0/golangci-lint

cmd/spire-agent/cli/run/run.go:23: File is not `goimports`-ed (goimports)

	"github.com/sirupsen/logrus"

Makefile:377: recipe for target 'lint-code' failed

@azdagron
Copy link
Member

goimports orders the imports alphabetically, which would place "github.com/spiffe/spire/cmd/spire-agent/cli/common" just after "github.com/sirupsen/logrus".

@amartinezfayo
Copy link
Member

Thoughts @evan2645 , @amartinezfayo , @APTy , @mcpherrinm? Any concern taking this potentially breaking change? What is your evaluation of the risk?

I'm OK with this change. I see the risk of breaking something really small and I think that it's good to have this fixed before 1.0.
I would make sure that we include this change in the CHANGELOG for the release so anyone updating is aware of the change.

@mcpherrinm
Copy link
Contributor

While I think this is a technically a breaking change, I think it's worth fixing this to a better default now.

@azdagron
Copy link
Member

@joewilliams , thanks for putting this together! We (maintainers) huddled and discussed this offline. I think we are ok accepting the small risk. If you wouldn't mind fixing up the sign-off on the commits (and maybe squash and rebase), we're ready to get this merged.

@joewilliams joewilliams force-pushed the sock_path branch 6 times, most recently from 767bd4d to 8790161 Compare July 23, 2020 23:48
update docs

fix tests

add cli defaults package

alphabetize imports

fix default socket path comment

alphabetize imports

Signed-off-by: Joe Williams <joe.williams@github.com>
@joewilliams
Copy link
Author

@azdagron okay, got things cleaned up.

@azdagron azdagron merged commit bdefb59 into spiffe:master Jul 24, 2020
@joewilliams joewilliams deleted the sock_path branch July 24, 2020 16:31
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.

None yet

4 participants