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

Add ni-apis as submodule and regenerate discovery client LabVIEW code #579

Merged
merged 23 commits into from
Aug 30, 2024

Conversation

pakdev
Copy link
Contributor

@pakdev pakdev commented Aug 28, 2024

  • This contribution adheres to CONTRIBUTING.md. (Required)
  • Automatically post PR comments with images for G code changes? (Recommended for small changes)

What does this Pull Request accomplish?

The discovery_service.proto in the ni-apis repo is more updated than the one here. For an upcoming change, I wanted the ServiceDescriptor.versions property. Thus, I added ni-apis/protobuf as submodules and regenerated the LabVIEW gRPC bindings from the new proto files.

Why should this Pull Request be merged?

Because our proto files should stay up-to-date and our discovery client LabVIEW code is now up-to-date.

What testing has been done?

Ran run_tests.vi

@dixonjoel
Copy link
Collaborator

I'm not super fluent with submodules, but I think everything under the ni-apis folder comes directly from that repo and is showing because you're adding the submodule? So I think we don't need to review anything under there?

@dixonjoel
Copy link
Collaborator

@pakdev Waiting to approve until the checks all pass.

@bkeryan
Copy link
Collaborator

bkeryan commented Aug 28, 2024

I'm not super fluent with submodules, but I think everything under the ni-apis folder comes directly from that repo and is showing because you're adding the submodule? So I think we don't need to review anything under there?

That looks wrong. When you added ni-apis as a submodule to measurement-plugin-python, GitHub displayed the submodule as a link and didn't display all of the files inside.

@pakdev
Copy link
Contributor Author

pakdev commented Aug 28, 2024

I'm not super fluent with submodules, but I think everything under the ni-apis folder comes directly from that repo and is showing because you're adding the submodule? So I think we don't need to review anything under there?

Right

@pakdev
Copy link
Contributor Author

pakdev commented Aug 28, 2024

On second look, it appears I shouldn't have pushed the contents. I'll fix that

@pakdev
Copy link
Contributor Author

pakdev commented Aug 29, 2024

Not sure what's going on with these tests - might need some help tomorrow morning. Not able to reproduce these 1003 errors in my branch...

@pakdev
Copy link
Contributor Author

pakdev commented Aug 29, 2024

@pakdev Waiting to approve until the checks all pass.

Passing now

This reverts commit 3314a9a.
@jasonmreding jasonmreding merged commit 7f1d20a into ni:main Aug 30, 2024
5 checks passed
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.

4 participants