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

Always refer to "finally" tasks 📖 #4090

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

bobcatfish
Copy link
Collaborator

@bobcatfish bobcatfish commented Jul 12, 2021

Changes

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 #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.:

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.

/kind documentation

Question for the reviewers: which approach do you think we should take:

  1. This one
  2. Rename uses of "finally tasks" to "final tasks" (i.e. dont try to force using an adverb as an adjective)
  3. Rename the "finally" section to "final"
  4. Support both "finally" and "final" in the pipeline spec
  5. Something else?

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • [n/a] Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/documentation Categorizes issue or PR as related to documentation. labels Jul 12, 2021
@tekton-robot tekton-robot requested review from afrittoli and a user July 12, 2021 23:15
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 12, 2021
@bobcatfish
Copy link
Collaborator Author

After writing this out im kinda leaning toward (2) instead - always saying "final" tasks, i.e. "final tasks" are tasks in the "finally section", but it seems like that might increase the confusion that @Srivaralakshmi was describing 🤔

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2021
@ghost
Copy link

ghost commented Jul 13, 2021

I lean towards (1), the change you've made. For the following reasons:

  • It's the field name we've already got. While language is important I don't see a reason compelling enough to lean towards "final tasks" in our docs, support both "final" and "finally" in our yamls or push users through a painful field renaming.
  • "finally" might be an adverb but I've gotten pretty comfortable thinking of them as "Finally Tasks". A Proper Noun. In other words I now think of Finally Tasks as a specific named feature of Tekton Pipelines.
  • "Final tasks" is so ambiguous as to be effectively meaningless - I feel like use of this phrase in conversation or writing will always need clarifying clauses to specify author's intent. "Finally tasks" is unambiguous as to its meaning (once you know the features of tekton pipelines).

I realize these are all quite personal opinions, and informed by experience with the project rather than new user glasses. My 2 cents.

@ghost
Copy link

ghost commented Jul 13, 2021

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2021
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 13, 2021
@bobcatfish
Copy link
Collaborator Author

I've made one more tweak which might help: instead of saying "finally tasks" i've added code block markup: finally tasks - which might help make it a bit more clear that finally here is referring to a symbol in the pipeline spec vs trying to act as an adverb

Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are several spots with ``finally task instead of `finally` tasks

that is, there's double code markup -- around "finally" and around "finally tasks"

otherwise looks great 👍🏾

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2021
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreeing on (1) too 👼🏼

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 tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021
@bobcatfish
Copy link
Collaborator Author

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2021
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, sbwsg, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [jerop,sbwsg,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jerop
Copy link
Member

jerop commented Aug 12, 2021

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2021
@jerop
Copy link
Member

jerop commented Aug 12, 2021

fixes #4086

@tekton-robot tekton-robot merged commit edcaa97 into tektoncd:main Aug 12, 2021
@pritidesai
Copy link
Member

thanks a bunch @bobcatfish for making it read better 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
5 participants