Issue #167: curl step

[marcghaly]

Hi @brendanheywood,

This is PR for issue #167,

Regards,

Marc

--

Resolves #167

[brendanheywood]

@marcghaly can you please rebase this, was a few clashes. Comments in the pr

[marcghaly]

Hi @brendanheywood,

I have fixed issues above, path issue and download_one method issue which by default is not outputting proper response for put and post. It is rebased over latest master.

Regards,

Marc

[marcghaly]

Hi @keevan,

This is a PR for issue #167, as you had that under radar is it possible for you to have a look at this ?

Thank you

Regards,

Marc

[keevan]

@marcghaly Sure thing. I can take a look

[keevan]

Hey @marcghaly I've done a pass of the code. I haven't tested the functionality of the code yet but I'll do that shortly.

[keevan]

Doing a bit more functional testing (from the UI):

1. There is no mention of where the response is stored if a file destination is not set. This information could be useful if you're creating steps in order.
2. There should be some handling for dry mode, in particular dry mode shouldn't create any files even if the file path was set, and it shouldn't perform the cURL request if it's marked as having a side effect in configuration.

[brendanheywood]

There should be some handling for dry mode, in particular dry mode shouldn't create any files even if the file path was set, and it shouldn't perform the cURL request if it's marked as having a side effect in configuration.

To refine this a little, if curl downloads a file into the local scratch dir then this should NOT be considered having a side effect. It is not changing state outside of its own temporary memory and storages. If that file was then copied in another step to path which was outside the scratch dir then that would have side effects. So in theory the only time a curl step should have side effects is if the remote host is marked as changing state

@marcghaly this is a little tricky as the scratch directory pr hasn't landed yet:

https://github.com/catalyst/moodle-tool_dataflows/pull/252/files

Also if you are in dry run mode, and normally you are curling a remote api and it grabs a file which is then used in later steps, then our workflow will now abort in dry run mode when the later step can't find the file it needs. So what we need is an additional setting which is the contents of the file it will write in dry run mode when it will not actually do the real curl call. This can just be a textarea or a file upload which ever is easiest.

[keevan]

I'm having trouble referencing the response from a subsequent step for when the destination path is not set.

e.g. using the expression steps.curl.config.response

To ease with testing, are you able to export the dataflow you've been using to test with?

[marcghaly]

Hi @keevan,

Thank you for the review, I made appropriate changes - are you able to use data from a former step this way $this->enginestep->stepdef->get_variables()['steps'], not sure if this steps.curl.config.response is yaml expression in which case I am not used to, do you have an example ?

This is the yml file I am using :

name: test
steps:
  test_put_connector:
    name: 'test put connector'
    type: tool_dataflows\local\step\connector_curl
    config:
      curl: 'https://reqres.in/api/users'
      destination: ''
      headers: ''
      method: post
      rawpostdata: |
        {
            "name": "morpheus",
            "job": "leader"
        }
        
      timeout: '0'
  test_get:
    name: 'Test get'
    depends_on: test_put_connector
    type: tool_dataflows\local\step\connector_curl
    config:
      curl: 'https://reqres.in/api/users/2'
      destination: ''
      headers: ''
      method: get
      timeout: '0'

Regards,

Marc

[marcghaly]

Hi @keevan,

I have fixed points cited above could you have another look if possible ?

Thank you

Marc

[keevan]

Hi @marcghaly , one other thing. I would like each type of step to have at least one unit test of some sort. Are you able to include one for this step please? I'll continue the review now.

[keevan]

Also, can you please add some explainer/help text to the "headers" field I'm not sure what the format should be in. I assume it would be something like:

header-key: header value
another-key: 1234
...
etc.

[keevan]

What is the reason this field needs to be updated?

In either case, if it's needed I think the handling for this doesn't need to be here in particular, and can join the snippet further above where it's initially set. This should be deterministic before the curl is run, so should be further above before any actual handling.

[marcghaly]

If method is post or put it will have side effects but setting it above will prevent the execution of the curl request

[keevan]

Sorry I'm not following. I've tried moving the if block up and making it look like so, and it seems to work okay:

        $this->hassideeffect = !empty($config->sideeffects);
        if ($config->method != 'get') {
            $this->hassideeffect = true;
        }

The other question was whether or not $this->hassideeffect was used anywhere in this step in particular, since it's only used for display purposes and an example is located here https://github.com/catalyst/moodle-tool_dataflows/blob/MOODLE_35_STABLE/classes/local/step/flow_step.php#L109

I think if it can change, based on configuration values, then perhaps the has_side_effect function in base_step needs to be overriden in the connector_curl step, to include the extra checks. It doesn't seem like the block I mentioned above does anything in particular, but if you think it should could you please clarify?

[marcghaly]

Sorry for the confusion, you are right it is working well when moved up, I tried it with older code - it is not used anywhere else in this step, I am unsure about what checks need to be done and why it should be overridden though

[marcghaly]

Hi @keevan, I have added unit tests and fixed issues - just unsure concerning the API I am using, should it be like a "mock api" so it is always "online", this is also rebased over last master.

Regards

Marc

[keevan]

Hi @keevan, I have added unit tests and fixed issues - just unsure concerning the API I am using, should it be like a "mock api" so it is always "online", this is also rebased over last master.

Regards

Marc

Hi Marc, I noticed there are various unaddressed comments on the files tab. Can you please respond to those open issues.

Also regarding the API test. It's much more reliable (and fast) if we mock the response instead of relying on a third party. There might be examples in core that do this already. From a high level, I'm expecting a combination of test things like:

• is the method set being called correctly?
• is the response being stored as a file correctly?
• is the response being stored into a variable correctly (where it can be referenced from future steps)
• shape of input/output data if any (more generally speaking)
• And anything else you'd want to check if someone were to ask, "How do you know this step works correctly?"

[marcghaly]

Hi @keevan, I have updated unit tests let me know status concerning side effects issue

Regards,

Marc

[keevan]

HI @marcghaly, could you please

• update the PR description to have something like "Resolves Prod ready curl step #167" as this will ensure the linked issue is automatically closed off when this PR merges. Also it allows you to access this PR from the kanban board.
• resolve the version.php conflicts.
• Could you please add an @author tag to the curl connector file, as you've written the code and deserve the kudos
• there might be one extra point I've made prior to this comment, so if you could fix that up as well.

With regards the side effects setting of state. I think though it's not used currently, it might see some use later on, but it's quite harmless to leave in there. If anything, it would be moved to a has_side_effects function (overriding the base class), which will then affect how it displays in the UI, but that could be in a separate issue and addresssed when needed.

But for now, I'm quite happy with what it's currently demonstrating so if there are any bugs / issues once used with the json reader step then we'll iron those out then as well.

Thanks @marcghaly.

[marcghaly]

Hi @keevan, points above have been fixed - branch is rebased over latest main one.

Regards,

Marc

[keevan]

Looks good now, there'll be more adjustments made as we go along since there are other things available but this is good to be merged in.