Skip to content

Conversation

@KolbyML
Copy link
Member

@KolbyML KolbyML commented May 11, 2023

What was wrong?

fixes #573 (trin-cli removal as talked about in the meeting 5/11/2023)
fixes Trin so that it can compile to windows
supercedes #698 (structout to clap conversion for trin-types)
updates/makes a few fixes to the trin book for grafana setup

How was it fixed?

  1. removed json-rpc from trin-cli
  2. removed encodekey from trin-cli
  3. add cfg and add a few panics to clearly define what is not supported by windows so it can compile
  4. remove trin-cli, move Create Grafana dashboard to TrinConfig (as discussed in the meeting 5/11/2023), update trin-types from structopt to clap
  5. update Trin Book

I updated structopt to clap since it was more work to keep structopt then just use clap, there is a WIP PR for it but it has been stalled for 2 weeks now of lack of development

To-Do

  • Add entry to the release notes (may forgo for trivial changes)
  • Clean up commit history

@KolbyML KolbyML force-pushed the remove-cli-and-fix-windows-compilation branch from c3ffd5b to 6748c6b Compare May 11, 2023 23:50
@KolbyML KolbyML changed the title Remove cli, fix windows compilation, update trin-types to use clap Remove cli, fix windows compilation, move grafana create to TrinConfig, update trin-types to use clap May 11, 2023
@KolbyML KolbyML force-pushed the remove-cli-and-fix-windows-compilation branch 2 times, most recently from 4d3e4c0 to 67937f6 Compare May 12, 2023 00:44
@KolbyML KolbyML force-pushed the remove-cli-and-fix-windows-compilation branch from 67937f6 to 9a1c951 Compare May 12, 2023 00:47
@KolbyML KolbyML marked this pull request as ready for review May 12, 2023 00:56
@ogenev
Copy link
Member

ogenev commented May 12, 2023

I would like to see this split into separate PRs. Reading the PR title suggests that this should be at least 3 separate PRs.

We want to do this because it is a lot easier for reviewers to spot any issues when the PR length is shorter and when it relates only to a single context.

Sometimes of course, this is not possible because a single feature requires changes all over the place but in this case I clearly see 3 different contexts: 1. trin-cli depreciation/move grafana 2. Fix Windows compilation 3. Update trin-types to use clap.

@KolbyML
Copy link
Member Author

KolbyML commented May 12, 2023

I would like to see this split into separate PRs. Reading the PR title suggests that this should be at least 3 separate PRs.

We want to do this because it is a lot easier for reviewers to spot any issues when the PR length is shorter and when it relates only to a single context.

Sometimes of course, this is not possible because a single feature requires changes all over the place but in this case I clearly see 3 different contexts: 1. trin-cli depreciation/move grafana 2. Fix Windows compilation 3. Update trin-types to use clap.

if moving grafana is seperate from clap grafana won't work till that is in then. I will see what I can do

@KolbyML
Copy link
Member Author

KolbyML commented May 12, 2023

windows requires deprecation of json-rpc.
grafana requires clap for context

@KolbyML KolbyML marked this pull request as draft May 12, 2023 14:44
@KolbyML
Copy link
Member Author

KolbyML commented May 12, 2023

@ogenev @njgheorghita ok I split it into #737 #738 #739 #740 #741

#741 has to be reviewed/merged after #740

#737 #738 are the removal of json-rpc and encodekey so if we want we can go straight to #741 or review #737 #738 to take burden off #741

@KolbyML
Copy link
Member Author

KolbyML commented May 12, 2023

The PR has been split into 5 smaller PR's

@KolbyML KolbyML closed this May 12, 2023
@KolbyML KolbyML deleted the remove-cli-and-fix-windows-compilation branch January 22, 2025 07:51
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.

Deprecate trin-cli

2 participants