Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve gatherers documentation and add rhai example outputs #160

Merged
merged 2 commits into from
Jan 16, 2023

Conversation

fabriziosestito
Copy link
Member

@fabriziosestito fabriziosestito commented Jan 13, 2023

This PR improves gatherers documentation, spellcheck, and format and adds rhai example outputs in place of golang test links.

@coveralls
Copy link
Collaborator

coveralls commented Jan 13, 2023

Coverage Status

Coverage: 96.183%. Remained the same when pulling 2754d78 on add_gatherer_output_examples into 7dd39a9 on main.

@fabriziosestito fabriziosestito changed the title WIP Improve gatherers documentation and add rhai example outputs Jan 13, 2023
@fabriziosestito fabriziosestito marked this pull request as ready for review January 13, 2023 13:19
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Nice! I just found an inconsistency in hosts gatherer.

guides/gatherers.md Show resolved Hide resolved
guides/gatherers.md Show resolved Hide resolved
guides/gatherers.md Outdated Show resolved Hide resolved
guides/expression_language.md Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Looks great. Only a copy/paste error in the hosts gatherer.
Ready to merge once that is fixed

| :------------------------------------------------------------- | :----------------------------------------------------------------- |
| `cib.configuration` | complete cib configuration entry as a map |
| `cib.configuration.resources.primitive.0` | first available primitive resource |
| `cib.configuration.crm_config.cluster_property_set.0.nvpair.1` | second nvpair value from the first element of cluster_property_set |
Copy link
Contributor

Choose a reason for hiding this comment

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

If the cluster_property_set is a potential list, we will need to include it here:
https://github.com/trento-project/agent/blob/main/internal/factsengine/gatherers/cibadmin.go#L48

guides/gatherers.md Show resolved Hide resolved
Sample arguments:
| Name | Return value |
| :--------------------------- | :------------------------------- |
| `totem.token` | extracted value from the command |
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have the new implementation of this gatherer done, we should include new examples. @jamie-suse could you do that in an additional PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep @arbulu89, I can add examples featuring an output that contains a list.

guides/gatherers.md Outdated Show resolved Hide resolved
guides/gatherers.md Outdated Show resolved Hide resolved
@fabriziosestito fabriziosestito force-pushed the add_gatherer_output_examples branch 2 times, most recently from 62ae5d3 to 28577f7 Compare January 16, 2023 13:38
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Looks good!
Only a type with a quote and I think the value in the host gatherer should be a list

guides/gatherers.md Outdated Show resolved Hide resolved
guides/gatherers.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

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

Just that minor comment; looks good otherwise

guides/gatherers.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants