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

revise: clarifies language around the allowed number of sections in tasks and workflows #598

Closed

Conversation

claymcleod
Copy link
Collaborator

@claymcleod claymcleod commented Dec 12, 2023

This pull request clarifies the allowed number of member sections within task and workflow blocks.

Rendered task and workflow sections.

It wasn't clear to me whether I should update bump the patch version number myself or whether the maintainers of the spec would bundle this change in with others—let me know if you'd like me to do that.

Closes #594.

Checklist

  • Pull request details were added to CHANGELOG.md

@claymcleod claymcleod force-pushed the spec/clarify-number-of-sections branch 5 times, most recently from b50b258 to 52addde Compare December 12, 2023 04:03
@claymcleod claymcleod force-pushed the spec/clarify-number-of-sections branch 2 times, most recently from b7c2883 to df11c9c Compare December 12, 2023 13:51
@patmagee
Copy link
Member

@jdidion For clarifications like this, should we make them against 1.1.2? or against 1.1?

@jdidion
Copy link
Collaborator

jdidion commented Dec 16, 2023

@claymcleod this is a great PR, thanks!

The only question is where to merge. Historically, our policy has been not to update a spec once it has been released. We recently modified this to allow patch releases, and have just released 1.1.1.

If we want to do another patch release to 1.1 (i.e., 1.1.2), then we should create a new branch for that (wdl-1.1.2), and this PR should target that branch. Otherwise, this PR should target wdl-1.2.

If it is decided that this branch should target wdl-1.2, then it will need to be updated to recognize the new requirements and hints section, as well as the deprecation of runtime.

In my opinion, we should get in the habit of making frequent patch releases (perhaps on a quarterly cadence, assuming there are any patches to make). So I suggest option 1 (targeting a new wdl-1.1.2 branch). Keep in mind that whenever a patch release is made, it is merged into the minor release branch, and then the next minor release branch will need to be rebased. In this case: merge wdl-1.1.2 into wdl-1.1, then rebase wdl-1.2 to wdl-1.1@HEAD.

Would appreciate feedback on this from others before going forward.

@geoffjentry
Copy link
Member

+1 to 1.1.2

@claymcleod
Copy link
Collaborator Author

Yeah, I'm also in favor of 1.1.2.

claymcleod added a commit to claymcleod/wdl that referenced this pull request Dec 16, 2023
@claymcleod claymcleod force-pushed the spec/clarify-number-of-sections branch from caea169 to 73b0033 Compare December 16, 2023 20:48
claymcleod added a commit to claymcleod/wdl that referenced this pull request Dec 16, 2023
@jdidion jdidion changed the base branch from wdl-1.1 to wdl-1.1.2 December 17, 2023 04:35
@jdidion
Copy link
Collaborator

jdidion commented Dec 17, 2023

Moved to wdl-1.1.2

@jdidion jdidion self-requested a review December 17, 2023 23:14
Copy link
Collaborator

@jdidion jdidion left a comment

Choose a reason for hiding this comment

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

Thanks @claymcleod. Overall I think this is great; however, I don't think we should be adding style recommendations here. Note that I haven't gone through the spec thoroughly to make sure I'm being consistent with this sentiment, but if there are places in the spec already where we're making stylistic recommendations I'd also advocate for moving those to a separate style guide.

@claymcleod claymcleod changed the base branch from wdl-1.1.2 to wdl-1.2 March 23, 2024 03:39
claymcleod added a commit to claymcleod/wdl that referenced this pull request Mar 23, 2024
@claymcleod claymcleod force-pushed the spec/clarify-number-of-sections branch from 73b0033 to 5cab466 Compare March 23, 2024 03:45
Previously, the number of sections within `task` and `workflow` blocks
was implied rather than explicit. This commit introduces clear, concise
language describing the number of sections allowed and whether each
section is required/optional. Furhter, some housekeeping items were
included to make the editted text more consistent.

Also included in this commit (these changes were squashed):

* revise: adds suggested ordering to elements

This is based on @geoffjentry's suggestions on openwdl#598.

* revise: adds `requirements` and `hints`, removes ordering
  recommendations

Closes openwdl#594
@claymcleod claymcleod force-pushed the spec/clarify-number-of-sections branch from 5cab466 to 8000af5 Compare March 23, 2024 04:08
@claymcleod
Copy link
Collaborator Author

@jdidion all comments have been addressed, the branch target has been updated to wdl-1.2, and, I believe at least, the changes necessary to make this merge-able into wdl-1.2 have been made. Please take a look and let me know if we need anything else.

@jdidion
Copy link
Collaborator

jdidion commented Mar 27, 2024

@claymcleod Regarding the target branch, maybe there was a misunderstanding. I think we agreed to target wdl-1.1.2. Can you please change it back?

Otherwise it looks good and I'm prepared to merge it.

@jdidion jdidion changed the base branch from wdl-1.2 to wdl-1.1.2 March 27, 2024 18:50
@jdidion jdidion changed the base branch from wdl-1.1.2 to wdl-1.2 March 27, 2024 18:51
@jdidion jdidion added the K-clarification (Kind) Clarifications regarding the WDL specification. label Mar 28, 2024
@jdidion jdidion added this to the 1.1.2 milestone Mar 28, 2024
@jdidion
Copy link
Collaborator

jdidion commented Apr 8, 2024

Made these changes here #632 rather than trying to rebase

@jdidion jdidion closed this Apr 8, 2024
@claymcleod
Copy link
Collaborator Author

Hey John,

Apologies, I was planning on responding last week but then was out.

In short, I had interpreted this comment:

If it is decided that this branch should target wdl-1.2, then it will need to be updated to recognize the new requirements and hints section, as well as the deprecation of runtime.

To mean that it would be okay to merge into 1.2 if I was willing to do the work to update the sections to be compliant with 1.2. Originally, I had agreed with that, but then I found some time to just update it for 1.2.

I'm okay with it going into either 1.1.2 or 1.2 provided that we make sure the right content gets on the right branch. In this case, the text you had merged in contains references to the requirements and hints sections while referring to the runtime section as 🗑️. Ultimately, the best course of action is up to you.

@claymcleod claymcleod deleted the spec/clarify-number-of-sections branch November 16, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
K-clarification (Kind) Clarifications regarding the WDL specification.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Clarify which task elements and workflows are singular and which can be specified multiple times.
4 participants