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

Call provider API from controller workflows #464

Open
3 tasks
stefania-popescu opened this issue Jan 30, 2025 · 5 comments
Open
3 tasks

Call provider API from controller workflows #464

stefania-popescu opened this issue Jan 30, 2025 · 5 comments

Comments

@stefania-popescu
Copy link

stefania-popescu commented Jan 30, 2025

Overview

Currently, the argo workflows that manage CRUD operations for resources call the provider CLI during the provider step. Now it needs to be changed so that the workflow calls the provider-service API. Each CLI call should have a corresponding API call that needs to be configured.

There's an integration test that stubs out the (old) provider interface; when the provider-service API is called, the new provider interface will need to be stubbed out since the shape is slightly different.

Tasks

  • Create stubbed provider service for testing
  • Change provider CLI to call provider-service HTTP API
  • Remove Image field from Provider CRD and everywhere it's used in workflows (we don't need a different image per provider, just pass diff parameters to API call)
@chr12c chr12c mentioned this issue Feb 6, 2025
2 tasks
@chr12c
Copy link
Contributor

chr12c commented Feb 11, 2025

To call the provider service api, we need to be able to send a URL encoded id from the workflow.
The delete api in particular expects an encoded id since they could involve /.

There’s 2 options:

  • encode the URL from the workflow
  • encode the URL from the Go code.

I think most would agree that leaking business logic into a yaml file isn't ideal since it's easier to control and unit test if it was within the Go code. To do this we can initially introduce another input parameter into the delete-related workflow templates; this won't break anything because nobody will be using it.

@alexgeorgousis
Copy link
Contributor

To call the provider service api, we need to be able to send a URL encoded id from the workflow. The delete api in particular expects an encoded id since they could involve /.

There’s 2 options:

  • encode the URL from the workflow
  • encode the URL from the Go code.

I think most would agree that leaking business logic into a yaml file isn't ideal since it's easier to control and unit test if it was within the Go code. To do this we can initially introduce another input parameter into the delete-related workflow templates; this won't break anything because nobody will be using it.

What would the input parameter be? Also, is this another reason to keep the provider CLI and make the API request from there? Along with everything else we'll need to handle. Basically what I'm saying is:

I think most would agree that leaking business logic into a yaml file isn't ideal

Can we use this as an argument to have the entire API request logic happen inside the CLI?

@chr12c
Copy link
Contributor

chr12c commented Feb 11, 2025

The parameter is the encoded resource-id.

I don't think this is a reason to keep the CLI just because the CLI can technically make an API request. This doesn't count as a reason for keeping it if the entire CLI can be removed if the template can make the call. The logic of encoding the id can be completely encapsulated in the construction of the workflow itself, i.e. the workflow factory. This doesn't seem too difficult.

What is the issue with introducing another input parameter?

Having less overhead is better:

workflow factory -> workflow cli -> api
workflow factory -> workflow http

Not to mention argo has support for http templates

@alexgeorgousis
Copy link
Contributor

The parameter is the encoded resource-id.

Ahh yes that makes sense 👍

I don't think this is a reason to keep the CLI just because the CLI can technically make an API request. This doesn't count as a reason for keeping it if the entire CLI can be removed if the template can make the call. The logic of encoding the id can be completely encapsulated in the construction of the workflow itself, i.e. the workflow factory. This doesn't seem too difficult.

Yep, all I meant was, if making the call needs adding business logic to the WF, then we should consider moving it to the CLI.

What is the issue with introducing another input parameter?

No issue, sounds good 👍

Having less overhead is better:

workflow factory -> workflow cli -> api workflow factory -> workflow http

Not to mention argo has support for http templates

Yep agreed 💯 I don't actually know how much logic it takes, if it's very minimal then defo remove the CLI 👍

@grahamia
Copy link
Contributor

The parameter is the encoded resource-id.

I don't think this is a reason to keep the CLI just because the CLI can technically make an API request. This doesn't count as a reason for keeping it if the entire CLI can be removed if the template can make the call. The logic of encoding the id can be completely encapsulated in the construction of the workflow itself, i.e. the workflow factory. This doesn't seem too difficult.

What is the issue with introducing another input parameter?

Having less overhead is better:

workflow factory -> workflow cli -> api workflow factory -> workflow http

Not to mention argo has support for http templates

Yes, we should be using the built in http workflow. It is quite standard that if you know the parameter you are sending could contain special characters then you should encode it to ensure no adverse side effects. This was what we discussed when we realised on testing the api call and added the decoder into the api that we would encode in the workflow. Agree it would be nice to unit test but think this will just need to be tested in the decoupled tests.

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

No branches or pull requests

4 participants