Revert "Revert "Remove buildir flag reloaded"" #389
Conversation
local/network.go
Outdated
| // remove old value if present, to look for vm binaries in new binary path location | ||
| delete(nodeConfig.Flags, PluginDirKey) |
There was a problem hiding this comment.
This seems pretty jank can't the Flags be defined by the user? We should definitely be keeping this override if the user requests it... right?
There was a problem hiding this comment.
Yup flags were possibly defined by the user when first created the node. In this case I assumed the user trying to restart a node by specifying a new binary (usually a different avago version in tests), was expecting also for the plugin dir to automatically change. I made this as I realised restart was lacking plugin dir param available. The other and probably cleaner option would be to add that plugin dir param to this function.
There was a problem hiding this comment.
Added the extra plugin param to RestartNode. Also to AddNode.
| tags: | ||
| - "*" |
There was a problem hiding this comment.
I don't know why this would be changed in this PR.
There was a problem hiding this comment.
I don't exactly remember. I think we had an issue with the tag name not passing if those lines where included. I believe @dasconnor made the suggestion to me
There was a problem hiding this comment.
My guess here is that it was to avoid goreleaser from performing a release when the tag was made
There was a problem hiding this comment.
But then we can't publish a release by tag? Is this the desired behavior?
There was a problem hiding this comment.
PM with @dasconnor give no conclusion on the origin of this haha including it again
| tags: | ||
| - "*" |
There was a problem hiding this comment.
But then we can't publish a release by tag? Is this the desired behavior?
| @@ -458,7 +458,7 @@ func (ln *localNetwork) restartNodesWithWhitelistedSubnets( | |||
| delete(nodeConfig.Flags, config.WhitelistedSubnetsKey) | |||
|
|
|||
| ln.log.Info("removing and adding back the node for whitelisted subnets", zap.String("node-name", nodeName)) | |||
| if err := ln.restartNode(ctx, nodeName, "", "", nil, nil, nil); err != nil { | |||
| if err := ln.restartNode(ctx, nodeName, "", "", "", nil, nil, nil); err != nil { | |||
There was a problem hiding this comment.
Note, not to be addressed in this PR but possibly create a ticket: These naked parameters are close to impossible to reason about what each means. I think we should not do this?
rpcpb/rpc.proto
Outdated
| @@ -381,6 +383,9 @@ message AddNodeRequest { | |||
| // If specified, will create a file "subnetid.json" under subnets config dir with | |||
| // the contents provided here. | |||
| map<string, string> subnet_configs = 6; | |||
|
|
|||
| // Plugin dir from which to load all custom VM executables. | |||
| optional string plugin_dir = 7; | |||
There was a problem hiding this comment.
Why are we marking this as optional? optional only needs to be used when there is a difference between the default value "" and no provided value nil. But they seem to be handled the same way. (To be clear, adding this without the optional keyword is still backwards compatible.
There was a problem hiding this comment.
You are so right. This was technical debt. I just copy/paste from Start prototype. Changed so. And added ticket for replacing all:
#391
server/server.go
Outdated
| @@ -773,10 +767,12 @@ func (s *server) AddNode(_ context.Context, req *rpcpb.AddNodeRequest) (*rpcpb.A | |||
| } | |||
| } | |||
|
|
|||
| nodeFlags[PluginDirKey] = req.GetPluginDir() | |||
There was a problem hiding this comment.
Should this only be populated if GetPluginDir() returns a non-empty string?
There was a problem hiding this comment.
ok that is more consistent
No description provided.