fix issue 2304: ux improvements #2317
Conversation
There was a problem hiding this comment.
Thanks for the PR!
Since those are breaking changes, I've changed the base branch to next
Also, given that we will eventually support running distributed PoSt on workers, we could also rename lotus-seal-worker to just lotus-worker here, to keep those changes in one batch.
cmd/lotus-seal-worker/main.go
Outdated
| @@ -36,7 +36,7 @@ import ( | |||
|
|
|||
| var log = logging.Logger("main") | |||
|
|
|||
| const FlagStorageRepo = "workerrepo" | |||
| const FlagWorkerRepo = "workerrepo" | |||
There was a problem hiding this comment.
Following the pattern from miner-repo, this could be worker-repo
There was a problem hiding this comment.
Thanks!
Probably want @travisperson to have a look at these changes too before we merge
Co-authored-by: Anton Evangelatov <anton.evangelatov@gmail.com>
There was a problem hiding this comment.
Thanks for taking this one!
The only thing I think we need to discuss further is if we should also move the code directories for lotus-storage-miner, lotus-seal-worker.
It will remove some history but that is what git log --follow is for.
This is a big breaking change, and doing it right before the testnet reset is a large risk as a lot of peoples infrastructure and deployments are going to change as a result of this. We should announce it in the community slack, and try to get this in for the next butterfly network reset when the new specs-actors lands.
I think we should also try to support a deprecation period for the flags and environment variables. This is pretty easy to do with our cli tool. The other option is to break fast and break hard during the buttefly networks tests so people are very much aware of the change prior to the testnet reset.
We could run the deprecation period through the testnet reset and do it afterwards. I would very much be interested in hearing from the community. I will post this PR in the slack to get some more eyes on it.
cmd/lotus-seal-worker/main.go
Outdated
| Name: FlagStorageRepo, | ||
| EnvVars: []string{"WORKER_PATH"}, | ||
| Name: FlagWorkerRepo, | ||
| EnvVars: []string{"LOTUS_WORKER_PATH"}, |
There was a problem hiding this comment.
We can support multiple environment variables and add the old flag to the Aliases to allow for a deprecation period. We can then use the urfave.cli BeforeFunc to add a deprecation warning for each environment variable.
We should strive to ensure that nothing breaks outside of the binary names for a short period.
| @@ -83,7 +83,7 @@ func envForRepo(t repo.RepoType) string { | |||
| case repo.FullNode: | |||
| return "FULLNODE_API_INFO" | |||
| case repo.StorageMiner: | |||
| return "STORAGE_API_INFO" | |||
| return "MINER_API_INFO" | |||
There was a problem hiding this comment.
To fully support deprecation we'd need to figure out how to handle this stuff as well. This only returns the used value (which is fine), but we would need to handle both in the GetAPIInfo function where the os.LookupEnv occurs.
@magik6k might have some ideas where, but an off the cuff solution would be to change both of these flagForRepo and envForRepo to return a slice. We use the first element for when we print the values, but search for both when we consume them (GetAPIInfo).
There was a problem hiding this comment.
how about add flagForRepoDeprecation and envForRepoDeprecation func? And delete them after deprecation period. Then it won't change exist function.
There was a problem hiding this comment.
Oh yes, that sound like a better idea
|
@deaswang there some more find and replace I think that needs to occur: Terms: These appear in the documentation site |
|
|
||
| install-chainwatch-service: chainwatch | ||
| install -C ./chainwatch /usr/local/bin/chainwatch | ||
| install -C -m 0644 ./scripts/chainwatch.service /usr/local/lib/systemd/system/chainwatch.service | ||
| mkdir -p /etc/lotus |
There was a problem hiding this comment.
for EnvironmentFile=-/etc/lotus/chainwatch.env in lotus-chainwatch.service.
Fix issue: #2304