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

Rename the 'finally' key to 'final' in the specification and while still allowing 'finally' to help the users who already have this key as part of their specification #4086

Closed
Srivaralakshmi opened this issue Jul 12, 2021 · 12 comments · Fixed by #4090
Assignees
Labels
area/api Indicates an issue or PR that deals with the API. kind/documentation Categorizes issue or PR as related to documentation. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@Srivaralakshmi
Copy link

Expected behaviour:

Actual behaviour: At present the name used to represent the final set of tasks at the end of the pipeline is represented as finally in the specification: https://github.com/tektoncd/pipeline/blob/main/docs/pipelines.md#adding-finally-to-the-pipeline

Reason for this proposal:

  • The word 'final' is an adjective that is describing the noun 'task', just like the term 'conditional' task (which is deprecated now and is replaced with 'when' expression).
  • 'finally' is an adverb and when we use it to describe 'task', it sounds incorrect and weird.
  • The docs mention both terms and switches between: 'final' task and 'finally' task, in multiple places (please see: https://github.com/tektoncd/pipeline/blob/main/docs/pipelines.md#adding-finally-to-the-pipeline). This is very confusing even for a new developer.
  • We can have final tasks and not finally tasks. We need to use one terminology consistently. Also the terminology must be correct both technically and from the language perspective as well.
@Srivaralakshmi Srivaralakshmi added the kind/bug Categorizes issue or PR as related to a bug. label Jul 12, 2021
@Srivaralakshmi
Copy link
Author

@nikhil-thomas PTAL at this ^ issue, comment on the issue, and kindly tag others. This will help drive some discussion. Thank you!

@bobcatfish
Copy link
Collaborator

hey @Srivaralakshmi - thanks for opening this issue.

The motivation for the term "finally" was inspiration from languages like python which have "try/catch/finally" syntax. This design doc has some more context around the design. What our current syntax is trying to say is:

  1. First, run all these tasks (tasks section - which actually is a DAG of tasks, you could argue we could have a better name for this as well)
  2. Finally, after that's done, whether it succeed or not, run these other tasks (finally section)

It sounds like one thing we could certainly do in the short term is to consistently use "finally tasks" in our docs instead of sometimes saying "final tasks".

Beyond that, could you explain a bit more about the problems this confusion is causing? If this is a matter of having to initially understand what 'finally' means, but users are able to use the feature after that, i'm not sure that's enough motivation to change the name of the field. But if we are seeing something like: folks are accidentally using final and then getting bizarre behavior, such as the section being silently ignored, then maybe we need to add some additional verification.

@bobcatfish bobcatfish self-assigned this Jul 12, 2021
@bobcatfish bobcatfish added area/api Indicates an issue or PR that deals with the API. kind/documentation Categorizes issue or PR as related to documentation. triage/needs-information Indicates an issue needs more information in order to work on it. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jul 12, 2021
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Jul 12, 2021
This commit changes all references to "final tasks" to "finally tasks".
We were using a mix of "final tasks" and "finally tasks" to refer to
these tasks, so this commit is making us consistently use one version.
However "finally tasks" is less grammatically correct that "final tasks"
because finally is an adverb - "finally tasks" only makes sense if
"finally tasks" is considered short for "finally tasks in a pipeline".
Another option sould be to go the other way and always use "final tasks"
(and never say "finally tasks"). However if we do this, @Srivaralakshmi
pointed out in tektoncd#4086 that
this may make it confusing in that reading something like 'the final tasks'
may make a user expect to see a seciton in the pipeline called `final`
instead of the actual section `finally`.

We have another option which is to consider `finally`- an adverb - a
bad name for that section of the Pipeline spec and change it to `final`.
My understanding of the syntax is a bit different, e.g.:
```yaml
spec:
  tasks:
    - name: tests
      taskRef:
        name: integration-test
  finally:
    - name: cleanup-test
      taskRef:
        name: cleanup
```
I understand the finally section to be saying: once all of the above is
done, finally run clean-test, and not to say "cleanup-test is a finally
kind of task". However that would be a big change (tho pre-v1 would be
the time to do it!) and `finally` is consistent with how this kind of
functionality is expressed in languages like Python.

The last option would be to support both final and finally but that
feels to me like it would be even more confusing.
@bobcatfish bobcatfish added the kind/bug Categorizes issue or PR as related to a bug. label Jul 12, 2021
@bobcatfish
Copy link
Collaborator

@Srivaralakshmi @nikhil-thomas I've added this to the next API working group agenda as well to discuss

bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Jul 13, 2021
This commit changes all references to "final tasks" and "finally tasks"
to `finally` tasks".  We were using a mix of "final tasks" and "finally
tasks" to refer to these tasks, so this commit is making us consistently
use one version.
"finally tasks" is less grammatically correct that "final tasks"
because finally is an adverb - "finally tasks" only makes sense if
"finally tasks" is considered short for "finally tasks in a pipeline".
We're working around this by using code block syntax to indicate that
`finally` is a symbol in the Pipeline spec, and not just the adverb
"finally".

Another option sould be to go the other way and always use "final tasks"
(and never say "finally tasks"). However if we do this, @Srivaralakshmi
pointed out in tektoncd#4086 that
this may make it confusing in that reading something like 'the final tasks'
may make a user expect to see a seciton in the pipeline called `final`
instead of the actual section `finally`.

We have another option which is to consider `finally`- an adverb - a
bad name for that section of the Pipeline spec and change it to `final`.
My understanding of the syntax is a bit different, e.g.:
```yaml
spec:
  tasks:
    - name: tests
      taskRef:
        name: integration-test
  final:
    - name: cleanup-test
      taskRef:
        name: cleanup
```
However that would be a big change (tho pre-v1 would be
the time to do it!) and `finally` is consistent with how this kind of
functionality is expressed in languages like Python.
(Also I understand the finally section to be saying: once all of the above is
done, finally run clean-test, and not to say "cleanup-test is a finally
kind of task".)

The last option would be to support both final and finally but that
feels to me like it would be even more confusing.
@rkratky
Copy link

rkratky commented Jul 15, 2021

@bobcatfish,

Beyond that, could you explain a bit more about the problems this confusion is causing?

Having the term "finally tasks" in docs is confusing because it sounds incorrect (it is incorrect, unless you know to read 'finally' as an adjective, which is very unintuitive). This makes the user (unless they're already familiar with the history) look for an explanation ("Is it a mistake?", "Is it correct but strange?"). Therefore, the docs would need to provide an explanation, which adds unnecessary complexity (and degrades overall user experience).

Another option would be to always use a roundabout way of referring to this, such as "tasks in the finally section". That would sidestep the problem.

@afrittoli
Copy link
Member

@bobcatfish,

Beyond that, could you explain a bit more about the problems this confusion is causing?

Having the term "finally tasks" in docs is confusing because it sounds incorrect (it is incorrect, unless you know to read 'finally' as an adjective, which is very unintuitive). This makes the user (unless they're already familiar with the history) look for an explanation ("Is it a mistake?", "Is it correct but strange?"). Therefore, the docs would need to provide an explanation, which adds unnecessary complexity (and degrades overall user experience).

Another option would be to always use a roundabout way of referring to this, such as "tasks in the finally section". That would sidestep the problem.

That sounds like a viable option to me.
I definitely agree we should be consistent in the documentation - thanks @Srivaralakshmi for reporting the issue.
As long as we make the terminology clear, I think we could also talk about "final tasks" to intend "tasks in the finally section".

@Srivaralakshmi
Copy link
Author

Srivaralakshmi commented Jul 15, 2021

@rkratky and @afrittoli Thank you very much for the response and support.

@Srivaralakshmi
Copy link
Author

@bobcatfish Thanks for the response and also for adding this issue to the API working group agenda. FYI, I do not have access to this document; have sent a request.

@bobcatfish
Copy link
Collaborator

@Srivaralakshmi to get access to the doc you need to join the tekton-dev mailing list, more info on how to do that at https://github.com/tektoncd/community/blob/main/contact.md#mailing-list 🙏

@bobcatfish bobcatfish removed the kind/bug Categorizes issue or PR as related to a bug. label Jul 16, 2021
@bobcatfish
Copy link
Collaborator

There as some discussion in the API working group today (notes here) - seems like the general sentiment is to stick with finally tasks for now (and avoid renaming the API field), which #4090 reflects - hopefully this can be a step forward at least.

@Srivaralakshmi
Copy link
Author

@bobcatfish Thanks for the update.

bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Aug 11, 2021
This commit changes all references to "final tasks" and "finally tasks"
to `finally` tasks".  We were using a mix of "final tasks" and "finally
tasks" to refer to these tasks, so this commit is making us consistently
use one version.
"finally tasks" is less grammatically correct that "final tasks"
because finally is an adverb - "finally tasks" only makes sense if
"finally tasks" is considered short for "finally tasks in a pipeline".
We're working around this by using code block syntax to indicate that
`finally` is a symbol in the Pipeline spec, and not just the adverb
"finally".

Another option sould be to go the other way and always use "final tasks"
(and never say "finally tasks"). However if we do this, @Srivaralakshmi
pointed out in tektoncd#4086 that
this may make it confusing in that reading something like 'the final tasks'
may make a user expect to see a seciton in the pipeline called `final`
instead of the actual section `finally`.

We have another option which is to consider `finally`- an adverb - a
bad name for that section of the Pipeline spec and change it to `final`.
My understanding of the syntax is a bit different, e.g.:
```yaml
spec:
  tasks:
    - name: tests
      taskRef:
        name: integration-test
  final:
    - name: cleanup-test
      taskRef:
        name: cleanup
```
However that would be a big change (tho pre-v1 would be
the time to do it!) and `finally` is consistent with how this kind of
functionality is expressed in languages like Python.
(Also I understand the finally section to be saying: once all of the above is
done, finally run clean-test, and not to say "cleanup-test is a finally
kind of task".)

The last option would be to support both final and finally but that
feels to me like it would be even more confusing.
tekton-robot pushed a commit that referenced this issue Aug 12, 2021
This commit changes all references to "final tasks" and "finally tasks"
to `finally` tasks".  We were using a mix of "final tasks" and "finally
tasks" to refer to these tasks, so this commit is making us consistently
use one version.
"finally tasks" is less grammatically correct that "final tasks"
because finally is an adverb - "finally tasks" only makes sense if
"finally tasks" is considered short for "finally tasks in a pipeline".
We're working around this by using code block syntax to indicate that
`finally` is a symbol in the Pipeline spec, and not just the adverb
"finally".

Another option sould be to go the other way and always use "final tasks"
(and never say "finally tasks"). However if we do this, @Srivaralakshmi
pointed out in #4086 that
this may make it confusing in that reading something like 'the final tasks'
may make a user expect to see a seciton in the pipeline called `final`
instead of the actual section `finally`.

We have another option which is to consider `finally`- an adverb - a
bad name for that section of the Pipeline spec and change it to `final`.
My understanding of the syntax is a bit different, e.g.:
```yaml
spec:
  tasks:
    - name: tests
      taskRef:
        name: integration-test
  final:
    - name: cleanup-test
      taskRef:
        name: cleanup
```
However that would be a big change (tho pre-v1 would be
the time to do it!) and `finally` is consistent with how this kind of
functionality is expressed in languages like Python.
(Also I understand the finally section to be saying: once all of the above is
done, finally run clean-test, and not to say "cleanup-test is a finally
kind of task".)

The last option would be to support both final and finally but that
feels to me like it would be even more confusing.
@jerop
Copy link
Member

jerop commented Aug 12, 2021

fixed in #4090

/close

@tekton-robot
Copy link
Collaborator

@jerop: Closing this issue.

In response to this:

fixed in #4090

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jerop jerop linked a pull request Aug 12, 2021 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue or PR that deals with the API. kind/documentation Categorizes issue or PR as related to documentation. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants