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

Authentication are not referenceable #904

Closed
cdavernas opened this issue Jun 27, 2024 · 14 comments · Fixed by #908
Closed

Authentication are not referenceable #904

cdavernas opened this issue Jun 27, 2024 · 14 comments · Fixed by #908
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 seems off:

Authentication are not referenceable

What you expected to be:

Authentications defined at top level should be referenceable

Anything else we need to know?:

After discussing the issue with @matthias-pichler-warrify, and well aware than the reference system previously in place was a PITA (oneOf[AuthenticationDefinition, string]), the proposal is something like the following:

use:
  authentications:
    basicTest:
      basic:
        username: test
        password: test
do:
  - getPetById:
      call: openapi
      with:
        operationId: getPetById
        parameters:
          id: 69
        authentication: 
          use: basicTest

While enabling the same than before, it:

  • Avoids oneOf constructs
  • Makes consistent use of objects
  • Makes the DSL more explicit, fluent and ubiquitous

One a side note, we could instead of use name it ref or whatever. My strong preference goes to use, which is more fluent, understandable, linguistical, is an imperative verb, and is not an abbreviation.

@cdavernas cdavernas added change: feature New feature or request. Impacts in a minor version change area: spec Changes in the Specification labels Jun 27, 2024
@cdavernas cdavernas added this to the v1.0.0-alpha3 milestone Jun 27, 2024
@cdavernas cdavernas self-assigned this Jun 27, 2024
@fjtirado
Copy link
Collaborator

I would also add as an argument in favour of this proposal (at the end oneof has to be managed in other areas, for example tasks) that this way all authentication information is located in the same section of the workflow definition, making it easier its review (or changing it, although I believe that authentication information is more "variable" than flow definition and that we should provide means to optionally read it from external properties files. That way, once the workflow definition is "stable", if the auth information change, you need to change the external property file, not the workflow definition itself)

@cdavernas
Copy link
Member Author

I would also add as an argument in favour of this proposal (at the end oneof has to be managed in other areas, for example tasks) that this way all authentication information is located in the same section of the workflow definition, making it easier its review (or changing it, although I believe that authentication information is more "variable" than flow definition and that we should provide means to optionally read it from external properties files. That way, once the workflow definition is "stable", if the auth information change, you need to change the external property file, not the workflow definition itself)

Totally agree! And even though users are still free to declare authentication inline (as I believe we shouldn't impose our will), we should probably insist, in the documentation, that the recommended way is to place them at top level instead, for all the good reasons you just provided.

@cdavernas
Copy link
Member Author

we should provide means to optionally read it from external properties files

Yes, totally agree, too. One way to deal with it so far is using secrets, but it might not be enough. Maybe we could add an external resource, or an endpoint property?

@fjtirado
Copy link
Collaborator

@cdavernas As we have secrets, we might add props (acutally in Kogito secrets is used both for secrets and properties)

@cdavernas
Copy link
Member Author

cdavernas commented Jun 27, 2024

@fjtirado so you would like to add a keyword that contains, just as const in previous versions, keyed values that you can they use with runtime expressions? And that can possibly be loaded from an external file?

If I understood correctly your proposal, I'm definitely up for it. I would however avoid the suggested abbreviation and change itto: properties, globals, variables or something of the like.

Could you create the issue to tackle that proposal, and the subsequent PR?

@fjtirado
Copy link
Collaborator

@cdavernas Any of your proposed names is fine for me :), pick the one you prefer ;)

@cdavernas
Copy link
Member Author

Chat GPT feedback on the question:

In the context of the Serverless Workflow DSL, the name for the argument containing declarative data for a workflow instance should ideally reflect its purpose and usage clearly. Let's consider the options provided:

  1. Properties: This is a general term that could fit but might be too vague in some contexts.
  2. Variables: This term suggests data that can change, which might not fully cover the use case if the data is meant to be static or semi-static.
  3. Constants: This implies that the data is immutable, which may not be accurate if the data can change during the workflow execution.
  4. Settings: This term typically refers to configuration values that dictate how something operates, which could be relevant but might not be the most precise.
  5. Configuration: This is a strong candidate as it conveys that the data is used to configure or control the behavior of the workflow.

Given these considerations, Configuration stands out as a clear and appropriate name since it suggests that the data is used to set up or control the workflow's operation.

Alternative Suggestions

  • Context: This term implies the data relevant to the workflow instance and could cover both static and dynamic aspects.
  • Metadata: This could be a suitable term if the data is descriptive and used to define properties of the workflow.
  • Parameters: This term conveys that the data can be used as inputs or controls for the workflow's execution.
  • Attributes: This term can imply properties that define or describe the workflow instance.

Final Recommendation

Out of all these options, Configuration seems the most fitting because it clearly suggests the purpose of the data in controlling or defining the workflow's behavior. If further clarity is needed, a combination term like WorkflowConfiguration or InstanceConfiguration could also be considered to be more descriptive.


My vote goes to configuration as the name of the runtime expression argument, and configure for the top-level property used to specify the workflow's properties.

What do you guys think @ricardozanini @fjtirado @JBBianchi @matthias-pichler-warrify ?

@matthias-pichler
Copy link
Collaborator

matthias-pichler commented Jun 27, 2024

@cdavernas To clarify if I understand the proposal ... you are proposing to add:

document: {}
use:
  secrets:
    - foo
  configure:
    bar: 1
do:
  - task1:
       set:
         size: ${ $configuration.size }

@matthias-pichler
Copy link
Collaborator

matthias-pichler commented Jun 27, 2024

Constants: This implies that the data is immutable, which may not be accurate if the data can change during the workflow execution.

I think my understanding differs from GPTs here. In my head they would be immutable ... right?

I'd prefer constants especially because it's consistent with secrets

document: {}
use:
  secrets:
    - foo
  constants:
    bar: 1
do:
  - task1:
       set:
         size: ${ $constants.size }
         shape: ${ $secrets.foo }

@fjtirado
Copy link
Collaborator

@matthias-pichler-warrify
In my opinion constants are hardcoded values that you do not want to duplicate in several places, so you define them in the header of the definition and refer to them in the rest of the document (as you did in the previous comment) , while the name we are discussing refers to configuration values read from a external configuration source (it is up to every implementor to define the valid configuracion sources)

@matthias-pichler
Copy link
Collaborator

Ah ok I somehow missed the "loaded at execution start". But hat is optional is it? They might be defined in the definition as well right?

I mean to some extent they are still constant for the duration of the execution...

I think if we go with configuration I would actually also go with use/configuration and $configuration. Potentially config is also a common enough abbreviation (even outside of tech?) that we might also consider it? (No strong opinions here though)

@fjtirado
Copy link
Collaborator

@matthias-pichler-warrify
About config being constant (picky comment warning ;))
I would say that a particular runtime implementation might read config when it is needed. In such case, if the config is accessed within a loop, it can potentially return different values (its a pretty rare occurrence but potentially possible). My point is that config is not necessarily constant

@matthias-pichler
Copy link
Collaborator

@fjtirado That is actually a very good point 👍 and actually I would argue that the specification should clarify this! (Whether configuration values can change within one execution)

I have a preference for configuration staying constant for the duration of an execution. Otherwise it might cause weird config drift for long running executions

@fjtirado
Copy link
Collaborator

@matthias-pichler-warrify I will fight for implementation freedom ;)
Just kidding, I do not really have a strong opinion about that, I feel the important point is to have a standard construct to access external configutation.

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
3 participants