Skip to content

Conversation

@dixia
Copy link
Contributor

@dixia dixia commented Feb 25, 2020

No description provided.

@dixia dixia requested a review from bobgt February 25, 2020 08:07
@iamveritas
Copy link
Contributor

pls merge it to dev
from dev we merge to master periodically

@lparisc lparisc self-requested a review February 25, 2020 17:11
@@ -0,0 +1,81 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this whole file qualifies as a "how-to". Starting from "Running Nodeos" the file contains concepts and explainers that act as prerequisites to the actual how-to in docs/03_tutorials/monitoring-with-state-history-plugin.md. Moreover, I copied some sections from this file to the nodeos implementation section in the nodeos manual. We need to discuss where to place this content going forward keeping in mind that we need a clear separation between explainers, how-tos, and tutorials.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we need to discuss which part to move around, take off and update. Now it is just at the formatting stage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would adding the following content title makes sense? This is to remove the "how-to" idea and make it more generic.


content_title: Monitor State History
link_text: Monitor State History

@dixia

Comment on lines 2 to 7
[block:callout]
{
"type": "warning",
"body": "The resources listed below are developed, offered, and maintained by third-parties and not by block.one. Providing information, material, or commentaries about such third-party resources does not mean we endorse or recommend any of these resources. We are not responsible, and disclaim any responsibility or liability, for your use of or reliance on any of these resources. Third-party resources may be updated, changed or terminated at any time, so the information below may be out of date or inaccurate. USAGE AND RELIANCE IS ENTIRELY AT YOUR OWN RISK."
}
[/block]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to proper callout

Comment on lines 32 to 37
[block:callout]
{
"type": "warning",
"body": "These are in no particular order. Descriptions of each tool have been provided by the tool authors."
}
[/block]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to proper callout

Comment on lines 39 to 43
[block:api-header]
{
"title": "Elastic Search Plugin"
}
[/block]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to proper markdown element

Copy link
Contributor Author

@dixia dixia Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lparisc please check the updated files as it should be resolved by @bobgt's commits

Comment on lines 48 to 52
[block:api-header]
{
"title": "Kafka Plugin"
}
[/block]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to proper markdown element (fix the rest below)

@lparisc lparisc changed the base branch from master to develop February 26, 2020 17:09
@lparisc
Copy link
Contributor

lparisc commented Feb 26, 2020

pls merge it to dev
from dev we merge to master periodically

Github rebase to develop worked in this case as the were no docs differences between the master based branch and develop, besides the new files added, of course.

@iamveritas
Copy link
Contributor

pls merge it to dev
from dev we merge to master periodically

Github rebase to develop worked in this case as the were no docs differences between the master based branch and develop, besides the new files added, of course.

I don't follow entirely your message here...
no matter what, this is the standard procedure, we merge our feature branches to develop and from develop to master. no exception. ok, I might be to harsh here. maybe, for some exceptional case we will do it the other way around. not the case here.

@dixia
Copy link
Contributor Author

dixia commented Feb 27, 2020

Hi guys, @bobgt is working on markdown syntax but thanks for taking time to review
And as such beware it is still a draft PR

@sunshineveg
Copy link

I think y’all need a live conversation regarding this issue. Set up a meeting to discuss your concerns and come to a conclusion. Let’s support each other. Let me know what you conclude.

@lparisc
Copy link
Contributor

lparisc commented Feb 27, 2020

@iamveritas no need to throw that strawman. If you look closer, the target branch now reads develop not master (I rebased it, hence my comment) - this is an example where it didn't work (read my last comment there). Not suggesting to bypass standard procedure, just made @dixia a favor.

@dixia dixia requested a review from lparisc March 2, 2020 07:42
Copy link
Contributor

@bobgt bobgt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments.

@dixia
Copy link
Contributor Author

dixia commented May 21, 2020

given the some components change, this probably requires more rewritten

@bobgt
Copy link
Contributor

bobgt commented Aug 6, 2020

I think this is good to merge for now.

@iamveritas iamveritas marked this pull request as ready for review August 6, 2020 14:51
@bobgt
Copy link
Contributor

bobgt commented Aug 6, 2020

Hey @lparisc: Are you okay with the updates made to this PR? Any more changes/suggestions from your side before we merge it?

@lparisc
Copy link
Contributor

lparisc commented Aug 10, 2020

@bobgt I'll check it first thing Monday


[[info]]
|
This article assumes you have installed the EOSIO software and are familiar with using the EOSIO nodeos and cleos tools. It is recommended that you have completed [the Getting Started section](https://developers.eos.io/eosio-home/docs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@lparisc lparisc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing, I'm mot sure merging this content is the best course of action at the moment for the following reasons:

  • The content related to the state history plugin needs updating.
  • Most if not all of the links to various nodeos sections are broken.
  • The way state will be monitored in EOSIO 2.1 will change anyway with rodeos.
    Let me delve a bit more on this and get back with some options.

@bobgt bobgt marked this pull request as draft August 24, 2020 18:20
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.

6 participants