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

update protobuf example #9479

Merged
merged 3 commits into from
Aug 11, 2021
Merged

update protobuf example #9479

merged 3 commits into from
Aug 11, 2021

Conversation

Scuilion
Copy link
Contributor

Just a slight update to documentation.

If this needs a jira ticket I'm happy to create one.

configurations
schemas
tasks
[//containers/default]> ls
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution @Scuilion, but I'm not sure if this change is correct. I believe the intent of the original example is to change to the caches/___protobuf_metadata] context and then display all registered *.proto files. Your addition simply shows the result of executing ls in the root context, which isn't necessary to verify the presence of person.proto.

\cc @oraNod

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, thanks for the contribution @Scuilion! Ryan is correct in that the current change you're suggesting doesn't actually verify person.proto. That said the verification step does need to change because it was written before the schemas directory existed. ___protobuf_metadata is still there, but is hidden.

We could change this to cd schemas then ls which shows all the Protobuf schema files.

Copy link
Contributor

@ryanemerson ryanemerson left a comment

Choose a reason for hiding this comment

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

I'm not sure about the .proto documentation changes, but everything else looks good 👍

configurations
schemas
tasks
[//containers/default]> ls
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, thanks for the contribution @Scuilion! Ryan is correct in that the current change you're suggesting doesn't actually verify person.proto. That said the verification step does need to change because it was written before the schemas directory existed. ___protobuf_metadata is still there, but is hidden.

We could change this to cd schemas then ls which shows all the Protobuf schema files.

Scuilion and others added 2 commits August 11, 2021 09:45
…otobuf_schema.adoc

Co-authored-by: Don Naro <dnaro@redhat.com>
…otobuf_schema.adoc

Co-authored-by: Don Naro <dnaro@redhat.com>
@Scuilion
Copy link
Contributor Author

Thanks for the feedback.

What is the process of updating a PR? Do I need to create a new PR with a single commit? Squash the commits that are here?

Copy link
Contributor

@oraNod oraNod left a comment

Choose a reason for hiding this comment

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

thanks @Scuilion

@oraNod
Copy link
Contributor

oraNod commented Aug 11, 2021

Thanks for the feedback.

What is the process of updating a PR? Do I need to create a new PR with a single commit? Squash the commits that are here?

@Scuilion No need for a separate PR. I'll just squash and merge and add a commit message. Thanks for the docs contribution!

@oraNod oraNod merged commit cd3b1bb into infinispan:main Aug 11, 2021
tristantarrant pushed a commit that referenced this pull request Jun 7, 2022
Docs revision: CLI and contributor guide updates
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.

3 participants