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 DiscoveryService with support for TestHub #31

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Update DiscoveryService with support for TestHub #31

merged 3 commits into from
Feb 6, 2024

Conversation

nick-beer
Copy link
Collaborator

What does this Pull Request accomplish?

This PR adds a single RPC to discovery service that will eventually enable remote execution of services. This remote execution already works (but is disabled behind a feature toggle) in a branch off of ASW I will soon be opening as a PR.

Specifically, the mew RPC is EnumerateComputeNodes. The behavior of the API is to return a list of "compute nodes" that have registered themselves with a TestHub instance. A "compute node" is basically just another instance of Discovery Service running on another machine somewhere that has registered itself with TestHub. Included in the list is the "local" compute node - corresponding to this local instance of DiscoveryService. With this information, you can indicate to ResolveService that you want a particular service to be instanced and run on a particular "compute node".

Why should this Pull Request be merged?

This pull request is critical to the merging of our TestHub work into ASW. We need to be able to maintain this API in order to demo and work on features created last yaer (including remote execution of services).

What testing has been done?

In our branch off of ASW, this new RPC has been implemented server side and demo'd multiple times.

@bkeryan
Copy link
Collaborator

bkeryan commented Feb 5, 2024

This branch is out-of-date with the base branch
Merge the latest changes from main into this branch.

@nick-beer
Copy link
Collaborator Author

This branch is out-of-date with the base branch
Merge the latest changes from main into this branch.

@bkeryan - I'm not super comfortable with the github workflows - did I do it right?

Additionally, I'm surprised this is something you'd ask for? Is it not likely/common for a development branch to become out of date from the main branch? And, if so, isn't that fine so long as there are not merge conflicts? Just trying to understand...

@bkeryan
Copy link
Collaborator

bkeryan commented Feb 5, 2024

This branch is out-of-date with the base branch
Merge the latest changes from main into this branch.

@bkeryan - I'm not super comfortable with the github workflows - did I do it right?

Yes, pushing your changes to a personal fork of the repo is fine. By default, the PR build requires approval for external contributors. I granted you write access, so approval should not be required again. This also means you can push to a dev branch inside the ni-apis repo.

Additionally, I'm surprised this is something you'd ask for? Is it not likely/common for a development branch to become out of date from the main branch? And, if so, isn't that fine so long as there are not merge conflicts? Just trying to understand...

That's how we currently have the repo set up. If it becomes a problem, we can configure it to be less strict. https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging

@nick-beer nick-beer merged commit e1e73b9 into ni:main Feb 6, 2024
2 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.

3 participants