Adds support for external resources

[cdavernas]

Many thanks for submitting your Pull Request ❤️!

Please specify parts of this PR update:

• Specification
• Schema
• Examples
• Extensions
• Roadmap
• Use Cases
• Community
• TCK
• Other

Discussion or Issue link:

#691

What this PR does / why we need it:

Adds support for external resources, and removes the ability to set an uri for auth, constants, retries, secrets, functions, events, ...

[ricardozanini]

Seems great! Just two minor comments.

[tsurdilo]

thanks for the pr. think we need to still update roadmap.md and add a full sample in our examples.md for this please.
would be nice to have more of a description for this feature as its quite a change.

one thing I'm not a big fan of is how much "resources" abstracts from the workflow definition itself. looking at a wf def that contains one ore more resources i get really confused as to what these resources contain and how i can reference these things. I think it would be better (yes more verbose but still) to add resources per type, so for example:

    "functions": {
      "resources": [
        
        ],
      "inline": [
        
        ]
    },
    "timeouts": {
      "resources": [
        
        ],
      "inline": [
        
        ]
    },
    "errors": {
      "resources": [
        
        ],
      "inline": [
        
        ]
    }

(change "inline" to whatever you want to call it).
Imo this is much easier to manage from user point of view and to understand whats going on by looking at the dsl while still adding the requested/needed functionality. wdyt?

[cdavernas]

@tsurdilo I personally find that ugly, counterintuitive and not ubiquitous. Resources are external, and you shouldn't pay attention to them in most cases.

Plus, doing like so, you are exploding one of the motivation of the PR, as discussed in the weekly, which is to be able to reference in one doc multiple concepts at once.

Finally, it does need to kinduv look like a wf, because that's what this aims to achieve: provide all resources for workflows to run in a specified context

The PR is the exact result of the many discussions we all had on the subject, please let's not yet again go on sidetracks because some fail to see its use.

[tsurdilo]

if in the future we add a new definitions array like functions, timeouts etc
do we say users have to update their existing reaource files? they might have prod workflows running and cannot update them or re-validate. what would u recommend in that situation? i see the benefit od this just trying to see if we can define it differently maybe

[tsurdilo]

for the inline defs i showed you can point to same resources file in each def so could keep the single resource json that contains all. just this way it would be clear what that resource contains

[cdavernas]

if in the future we add a new definitions array like functions, timeouts etc
do we say users have to update their existing reaource files?

Users should be able to do whatever they want in that case. They can append new definitions, or just leave it like it was: there'd be nothing broken anyways. In case of specVersion mismatch though, there's a problem, but that applies to any reference anyways. This should be a runtime concern only.

[cdavernas]

for the inline defe i showed you can point to same resources file in each def so could keep the single resource json that contains all. just this way it would be clear what that resource contains

What you suggest is not clear at all IMHO and forces the users to uselessly copy the same external resource file amongst the multiple resources that might need it. This would make it painfull for everyone.

Again, the PR is the exact result of what was discussed and approved, I don't see the need to go over it once again.
The proposal is clear, ubiquitous and clean, and the alternatives you propose are not IMO, and do not fully address requirements.

As for your actual concern which is having the same concepts in two different places in the spec, I'd say that there is no duplicates: on one hand you have definitions of external resources (which, again, most users won't even pay attention to), and on the other hand you have the perfectly fine functions, events and whatnot properties. Those are ubiquitous, clean and straightforward and should IMO not be changed in any ways (aside from forbidding non-inline values)

[cdavernas]

just this way it would be clear what that resource contains

You have no control, spec-side, on what an external resource may or may not contain. At best, you have an idea at a certain point in time, thus the named refs, but that's about it. Why would use ask authors of workflows to duplicate the work that has already been declaratively done in the external resource? What's the gain, if any? If you want to know for sure what a resource contains, get and read it.

[fjtirado]

@cdavernas
Thanks a lot
Thats what we agreed in the last meeting and you reflected it perfectly in this PR.
I do not think we need to repopen a discussion that was apparently closed @tsurdilo
This covers a gap that we had in the current spec with minimal changes
Additionally, this removed the difficulties associate with one of.
And it is easily extensible if a new resource is added

[tsurdilo]

I do not think we need to repopen a discussion that was apparently closed

I dont think its ever too late to question any decisions with this project by anyone. Without questioning whats there now we would not need to make any more changes in that case, even for this pr, right ? :)

This spec is what it is because we discussed things over and over and over again to make it the best we can and this should continue imo.

We do not have a timeline set for 0.9 and there is no rush that i can see in order not to have time or opportunity to discuss this, especially if its project maintainers that would like an open discussion about their opinions.....

[fjtirado]

@tsurdilo
I could agree with you, if we have not invested a complete meeting discussing this and agree on this approach after more than hour of discussion.
In my opinion, once an agreement is reached, it should be respected by all the parts involved on it.
You comment about splitting the existing properties into two rather than using a new property was discussed during that meeting and it was decided (or that was my understanding) to go on with the separate property approach. .
Re-discussing this particular aspect again might be intepreted as breaking such agreement (I do not think this is your intention, but if you want to reopen the discussion, it will be great if you can provide new arguments, because as far as I recall this was already discussed in the exact same terms during the meeting). It would be different if we were discussing small details or new aspect that we missed, but thats not the case, we are just repeating the same discussion over and over.

[tsurdilo]

I'm not sure what you are trying to say but one thing is a discussion, another is reviewing a PR. Once you see the actual proposed impl in a pr and try to test it, questions can come up.

I would like to keep the discussions here related to the PR itself, anything else feel free to bring up in weekly meets or offline.

[ricardozanini]

I agree that we need to keep discussing the issues as they appear in a PR that can change the perspective.

Honestly, I don't see any changes that go against what we've discussed, and I think it's reasonable to raise questions.

Having said that, I believe the discussed plan still holds. As proposed, the resources attribute can reasonably solve this problem, whereas adding it to each definition can make it more lengthy and hard to maintain.

[fjtirado]

I'm not sure what you are trying to say but one thing is a discussion, another is reviewing a PR. Once you see the actual proposed impl in a pr and try to test it, questions can come up.

What I'm trying to say is that your proposal of splitting every property that can have a resource into two were already discussed in that meeting (now I have doubts we exactly discuss this)
Anyway, lets rediscuss, including two fields for every property (inline and external) has problems that Charles has already pointed out:

• It is too verbose and it is a lot of copy paste (I hope we agree copy paste is an issue) in the spec schema.
• It will force the user to have an external resource for every different type, making impossible to define one external resource property for every API you want to integrate. The main point of importing extenal resource is to avoid declaring things in several flow files accesing a certain API, but if, in order to invoke that api, you spread all the knowledge in a lot of files, you come back to the original issue (at least you are not defining functions twice, but you will have knowledge distributed into several files)
  But my main concern is that I hardly see any advantage over Charles proposal. Charles proposal still allows you to put every resource type into a different file (I do not recommend that though) and you can even put the resource type in your file name. So if a user has the issue you mention to suport your proposal, he can still fufill it with Charles approach. On the contrary, if we took your approach, there is no way the user can put all the resources related with an API into the same file. So, Charles proposal is more flexible.

[tsurdilo]

Please add entry to roadmap and if possible an example. Thnks.

[tsurdilo]

#701 (comment)

let me know when to review again

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Change request is still pending, even though AFAIK every single change request has been addressed

[cdavernas]

@ricardozanini Bump

[cdavernas]

Not surprisingly: stale after sitting for about 2 years

[ricardozanini]

@cdavernas can you fix the schemas/example CI? So we can merge?

[ricardozanini]

Closed as part of #843