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

Added a new rpc call workflow.GetWorkflowContextList to be used by boots #237

Merged
merged 3 commits into from
Aug 6, 2020
Merged

Added a new rpc call workflow.GetWorkflowContextList to be used by boots #237

merged 3 commits into from
Aug 6, 2020

Conversation

gauravgahlot
Copy link
Contributor

@gauravgahlot gauravgahlot commented Aug 6, 2020

Description

As the worker starts, Boots checks if there is a workflow defined for that worker. If yes, it will continue to PXE and it will not otherwise. (more context here).

The PR introduces a new RPC that can be used by Boots to get the workflow contexts without having to maintain a gRPC stream.

Why is this needed

Recently there have been changes in Tink, to allow a worker to maintain a gRPC stream with the server to get the workflows. The same RPC was being by boots.

How Has This Been Tested?

Still a WIP.

How are existing users impacted? What migration steps/scripts do we need?

No direct impact on users.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>
Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>
Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>
@gauravgahlot gauravgahlot self-assigned this Aug 6, 2020
@gauravgahlot gauravgahlot added this to In Progress in Issues List via automation Aug 6, 2020
@Cbkhare
Copy link
Contributor

Cbkhare commented Aug 6, 2020

Let me first test it with boots then merge it.

@Cbkhare Cbkhare self-requested a review August 6, 2020 08:37
@Cbkhare Cbkhare added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. fix-required labels Aug 6, 2020
@Cbkhare
Copy link
Contributor

Cbkhare commented Aug 6, 2020

changes done in accordance with #238

Copy link
Contributor

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

Can we write a unit test for this?! I would like to have a table test that insert a set of fixtures and checks what we expect. This is a crucial part of the application @gauravgahlot can you add that to this PR?

@Cbkhare
Copy link
Contributor

Cbkhare commented Aug 6, 2020

Tested the workflow execution using this PR. changes are working correctly.
For more ref follow tinkerbell/smee#64

@Cbkhare
Copy link
Contributor

Cbkhare commented Aug 6, 2020

Can we write a unit test for this?! I would like to have a table test that insert a set of fixtures and checks what we expect. This is a crucial part of the application @gauravgahlot can you add that to this PR?

@gianarb Will it be fine if we merge this PR and @gauravgahlot can add table test for it in a separate PR. To fix issues related to #222 , this is a kind of priority.

@gianarb
Copy link
Contributor

gianarb commented Aug 6, 2020

Yes I agree but let's get it done as soon as it is merged please

@gauravgahlot gauravgahlot added the ready-to-merge Signal to Mergify to merge the PR. label Aug 6, 2020
@mergify mergify bot merged commit 0f947d8 into tinkerbell:master Aug 6, 2020
Issues List automation moved this from In Progress to Just shipped Aug 6, 2020
@gauravgahlot gauravgahlot deleted the context-fix branch August 6, 2020 10:49
@Cbkhare
Copy link
Contributor

Cbkhare commented Aug 6, 2020

Yes I agree but let's get it done as soon as it is merged please

Thanks @gianarb ,

@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
No open projects
Issues List
  
Just shipped
Development

Successfully merging this pull request may close these issues.

Changes in boots to get WorkflowContextList from updated method in Tink
4 participants