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

Refactor external references #691

Closed
cdavernas opened this issue Sep 30, 2022 · 29 comments
Closed

Refactor external references #691

cdavernas opened this issue Sep 30, 2022 · 29 comments
Assignees
Labels
area: spec Changes in the Specification change: feature New feature or request. Impacts in a minor version change
Milestone

Comments

@cdavernas
Copy link
Member

What would you like to be added:

Refactor external references.

What I propose is:

  1. To define a resource object, such as the following, used to encapsulate external definitions:
auth:
  - name: foo_basic
    scheme: basic
    properties:
      username: username
      password: password
functions:
  - name: secured_function
    type: rest
    operation: https://foo.bar#baz
    authRef: foo_basic

In other words, an external function definition file would not contain an array of function definitions, but an object such as the above.

In addition, that would allow serverless-workflow compatible apps to provide a self-describing document making it ready to use in Serverless Workflow.

  1. Instead of having functions, events, etc. be of type oneOf<uri, functionArray>, they should be only of type array, as discussed in the meeting of the 27th of September, and in Function definition namespaces (Importing several function definition files) #676.
    A new imports (or resources) top-level properties would replace the need for uri-values, and would allow the import of multiple files instead of just one.
name: foobar
version: 0.1.0
specVersion: 0.9
imports:
  - namespace: foo
    uri: https://foobar.baz/sw.res
  - namespace: bar
    uri: https://bazfoo.bar/sw.res
states:
  - name: invoke_external_functions
    type: operation
    actions:
      - name: invoke_foo
        functionRef:
          refName: foo.greet
          arguments:
            greetings: Hello, Serverless Workflow!
     - name: invoke_bar
        functionRef:
          refName: bar.greet
          arguments:
            greetings: Hello, dear readers!
    end: true

Why is this needed:

Currently, external references are easy and simple to use, but unhappilly do not address all common/reasonnable use cases, as explained in #676. In addition, it forbids referencing in a safe manner external functions that define an authRef

@cdavernas cdavernas added change: feature New feature or request. Impacts in a minor version change area: spec Changes in the Specification labels Sep 30, 2022
@tsurdilo
Copy link
Contributor

In your example where is the refName: foo.greet function definition? is the function def named "greet" and you append the namespace to it?

@cdavernas
Copy link
Member Author

@tsurdilo yes, exactly: {namespace}.{functionName}

@tsurdilo
Copy link
Contributor

tsurdilo commented Sep 30, 2022

Thanks, yeah i put a comment on that associated issue..do really like it but then why not

 {namespace}.{stateName}
 {namespace}.{actionName}
 {namespace}.{eventDefName}
 ...

I think these type of changes are awesome but in the grand schema of things if added should be thought to be added to entire spec not just one piece at a time imo. we could find by looking at this on higher level more use to it than just function def and then realize its powerful and come up with approach so this can be added to everything

@cdavernas
Copy link
Member Author

Events, functions, authentications, retries and timeouts. For states, that makes less sense imho considering the ability to call subflows. So only those should be cobsidered "resources" imo

@tsurdilo
Copy link
Contributor

Yeah I understand, i think with this also comes a lot of possible issues on user level (possible typos since you have to define a namespace in each action, maybe a better approach would be to have a top-level namespace property instead and if user does not set it would default to "default" which is the base resource def name.

@tsurdilo
Copy link
Contributor

One more question, does this apply only for "rest" action type?
For everything else the resource / uri would be defined inside the offloaded json/yaml (openapi/asyncapi/grpc, etc)

@tsurdilo
Copy link
Contributor

If so I am very much not "game" with adding all this just for rest type as we as spec do prefer users to use the standards and not "rest"..I thought we adding rest only for small scale scenarios and tests for most part.

@cdavernas
Copy link
Member Author

No it should be for everything imo. There is no difference anyways.

@tsurdilo
Copy link
Contributor

Ok so can we see example with openapi where the uri is defined in the openapi def? I must be missing something

@cdavernas
Copy link
Member Author

cdavernas commented Sep 30, 2022

You can define a grpc function def with authRef

@cdavernas
Copy link
Member Author

It's defined in the external resource, as you would right now. It's just moved out the wf doc and into the external res, possibly with its referenced auths

@tsurdilo
Copy link
Contributor

tsurdilo commented Sep 30, 2022

Ah ok so here we are just talking about the actual uri resource to the openapi definition ok ok.

Yeah i think let's maybe look at having a "resourcesNS" top level property instead than having the do the weird lookup
of each function def by appending namespace pls, think that would make it easier on users.

@cdavernas
Copy link
Member Author

The whole point is to be able to have multiple namespaces, so a top level ns property is not gonna help, or I'm not getting what you mean.

@tsurdilo
Copy link
Contributor

Yes multiple namespaces but a single one would apply to the whole workflow definition when set, so you can have a wf exec in test env using ns A and in staging B if you wanted.

Also think that this can be implemented as an extension point, this is quite a big change imo to enforce namespace lookups from what we have now and if impls wanna use it just define extension point for it and set the mapping of like stateName->actionName to namespace there

@tsurdilo
Copy link
Contributor

tsurdilo commented Sep 30, 2022

I think often we forget that our extension mechanism is quite powerful in the spec :) Either way if we go with this lets make sure it applies to all "resources" and to not enforce user to define a namespace on each of the reference point in their wf def but allow it to be set globally, would make things easier imo

@cdavernas
Copy link
Member Author

No, it's meant to import multiple namespaces at the same time. For instance, myGreetingApp's and myLoggingApp's. And that has imo nothing to do with extensions, namespace is not the feature here. The feature is:

  1. To be able to reference multiple external files (and not just one like now, with no possibility to append your own)
  2. To be able to have external functions that can reference auth in doc, which is not possible either at the moment, making imports of external functions useless in prod

@cdavernas
Copy link
Member Author

cdavernas commented Sep 30, 2022

Plus, extension is what it is: duct and tape. Makes your definitions suddenly non portable, thus going against SW core principles

@ricardozanini
Copy link
Member

ricardozanini commented Oct 4, 2022

I agree with @cdavernas. This proposal deals with the discussion in #676 for all external resources we might have, adding the possibility to reuse the resources across the workflow definitions. Additionally, I'd say that if we make this move, it should be before 1.0.

It's a huge change but, at the same time, won't break anything since old definitions would be assumed to be in the default namespace. This change helps more complex use cases or environments with multiple definitions that can be reused among other definitions.

@fjtirado
Copy link
Collaborator

fjtirado commented Oct 5, 2022

@cdavernas
Thanks for the proposal.
I think the namespace separator should be : not . (basically because : is less likely to be part of a function name than .)
And thinking about making implementors life easier, probably we should add resource type to the import. This has the benefit of restricting (and setting) the resources that can be imported (related with one of the aspect of the the conversation you are having with Tiho)

@cdavernas
Copy link
Member Author

cdavernas commented Oct 7, 2022

I think the namespace separator should be : not . (basically because : is less likely to be part of a function name than .)

@fjtirado Hmmm, not fan of the : though. First because the dot is commonly used for namespaces, at least in Borland. Second because I associate that char as the version separator, which is usually its purpose. But that's just a matter of personal preference I guess.

And thinking about making implementors life easier, probably we should add resource type to the import.

Very good point!

@tsurdilo
Copy link
Contributor

tsurdilo commented Oct 7, 2022

extensions do not go "against" anything , they are part of spec. we seem to be moving in direction of "spec for specific runtimes" than "spec for all runtimes" and id like not to move in that direction. if you guys feel otherwise pls ping me we can discuss i guess

@cdavernas
Copy link
Member Author

cdavernas commented Oct 7, 2022

@tsurdilo I disagree TO. I believe we are actually just moving from a theoretical spec to an implemented one. During implementation, I believe we discover limitations/issues otherwise unseen. Not addressing those limitations/issues, especially unanimous ones, is gonna ensure that people are gonna move away or branching the spec out imho.

Anyways, this has little to do with runtime, it's a pragmatic, useless limitation of the spec to only be able to import one external file per functions, per example. It makes external references useless, dot, except for very basic demo use cases.

This proposal addresses those changes, makes it easier to implement the spec (for newcomers: we already did it) and allows complex external files with embedded auth, etc. What's not to like about it?

@tsurdilo
Copy link
Contributor

tsurdilo commented Oct 8, 2022

well as author of this spec i respectfully can say we have different opinions on this

@cdavernas
Copy link
Member Author

cdavernas commented Oct 8, 2022

As another author (and owner) of the spec, I'd like you to respectfully elaborate:

  1. You are cool with the fact we can just reference one doc per concept?
  2. You are cool just referencing unsecured actions?
  3. You are cool not being able to have two functions, coming from two external definitions with the same name? And having to dev some magic feature to check name duplication in unknown external files?
  4. You are cool having union types on functions, events, etc., which is a pain to implement in any non prototype language, and which add no value (cfr. other points)?

Well, to me it's a no to all answers, and that's the motivation of this suggestion as well as of @fjtirado and of @ricardozanini ones.

Now, while I perfectly understand you might not like the proposed changes (tastes and colors, after all), just please acknowledge those issues (and trust us on this), work your magic out and propose unanimous (non-extension/non-metadata based) alternatives ❤️!

@github-actions
Copy link

This issue 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
Copy link

github-actions bot commented Jan 8, 2023

This issue 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
Copy link

This issue 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
Copy link

This issue 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.

@cdavernas
Copy link
Member Author

cdavernas commented May 17, 2024

Closed as resolved by 1.0.0-alpha1, and therefore as part of #843

This was referenced May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: feature New feature or request. Impacts in a minor version change
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants