Hidden agent id flag #160
Hidden agent id flag #160
Conversation
arbulu89
force-pushed
the
hidden-agent-id-flag
branch
from
599e9e7 to
f55935f
Compare
arbulu89
force-pushed
the
hidden-agent-id-flag
branch
from
f55935f to
fcdf523
Compare
arbulu89
requested review from
CDimonaco,
dottorblaster,
nelsonkopliku,
EMaksy,
fabriziosestito,
jamie-suse and
rtorrero
| @@ -12,7 +13,7 @@ func NewAgentIDCmd() *cobra.Command { | |||
| Use: "id", | |||
| Short: "Print the agent identifier", | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| agentID, err := agent.GetAgentID() | |||
| agentID, err := agent.GetAgentID(afero.NewOsFs()) | |||
There was a problem hiding this comment.
This PR opens the need to refactor also id command I guess, because as of now it will always get the real agent id.
What I would expect is that when ./trento-agent id is executed it returns either what has been configured in agent's config file or the real agent id.
As an addition I wouldn't expect ./trento-agent id --force-agent-id=blaa, as it would be fancy 😅
Finally we'd need to take care of having the same defaults for both the commands, otherwise in one case viper would complain about not having some expected flags.
There was a problem hiding this comment.
I wouldn't go in this direction. This force-agent-id is a hidden flag for development/test purposes, and it shouldn't be used together with the id command, which is used to get the real id. It is only for the start function, which is the one that it makes sense.
Using ./trento-agent id --force-agent-id=blaa doesn't make any sense.
There was a problem hiding this comment.
Yeah true, that's just for test/dev so if that's how it has to be I can live with this little inconsistency 😉
Using ./trento-agent id --force-agent-id=blaa doesn't make any sense.
Yes, we're saying the same about this 😄
| ) | ||
|
|
||
| // MockMachineIDFile mocks the /etc/machine-id file to have a known value | ||
| func MockMachineIDFile() afero.Fs { |
There was a problem hiding this comment.
I find this a bit misleading as we're mocking the whole filesystem, not only the file 😅
There was a problem hiding this comment.
How would you call it?
I find the name together with the docstring pretty meaningful, but I wrote msyelf the function, so I'm biased
There was a problem hiding this comment.
I wouldn't call it 😜
However, I can think of MockFilesystemWithMachineID, MockFilesystemMachineID, which are by no means any better than MockMachineIDFile at the end 😅
So up to you.
Add a new hidden
agent-idflag to have the option to fake the agent id.This comes handy during development/testing, as you can have predefined IDs, for whatever need you have.
I have added some dependency injection with Afero to manage the filesystem in the
GetAgentIDfunction. Not so nice to be passing this value all over the places, but well, it makes testing way better.PD: Finally, I have changed the horrible testing of the
config_test.gofile using the teardown 🤦