-
Notifications
You must be signed in to change notification settings - Fork 155
fix(orchestrator): stop fetching workflow URI #1297
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
Conversation
@gciavarrini could you please help me review this PR? Especially the API part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, I think all the changes to the API are good.
You also took care of the unit test.
I have just a couple of minor comments
58668ce
to
56599c8
Compare
56599c8
to
aadfffa
Compare
|
@jkilzi PTAL |
LGTM |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
https://issues.redhat.com/browse/FLPATH-1044
We were fetching each workflow URI directly from the workflow service when loading the workflow table.
If one of the services is not responsive, the loading operation fails. Not to mention that it adds one extra request for each item in the table.
I've noticed that we were using the workflow URI only for authoring purposes so I'm removing this field in favor of the
format
field (json|yaml
), which can be extracted from the workflow source, i.e., does not depend on fetching the workflow service directly.I've also taken the opportunity to improve our API by simplifying some of the operations and removing others that are not being used anymore. There's still room for improvement and this PR is a proposal for a first step.