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

2022 Week 44 release #337

Closed
wants to merge 50 commits into from
Closed

2022 Week 44 release #337

wants to merge 50 commits into from

Conversation

mr-c
Copy link
Member

@mr-c mr-c commented Oct 31, 2022

No description provided.

dear-ore and others added 18 commits October 18, 2022 12:33
The command to install graphviz should be "sudo apt-get install graphviz" not "sudo apt get install graphviz".

Co-authored-by: Michael R. Crusoe <1330696+mr-c@users.noreply.github.com>
`python3 -m venv venv` instead of `python -m venv venv`
* modified quick-start.md
* Update prerequisites.md
* Update basic-concepts.md
* Update faq.md
* Update yaml-guide.md

Co-authored-by: Peter Amstutz <peter.amstutz@curii.com>
* fixed typos and punctuation errors

* Update best-practices.md

* Update best-practices.md

Co-authored-by: Peter Amstutz <peter.amstutz@curii.com>
* docker clarity

* typo fix

Co-authored-by: Peter Amstutz <peter.amstutz@curii.com>
Co-authored-by: Peter Amstutz <peter.amstutz@curii.com>
corrected typos, corrected punctuations, rephrased  sentences, added a link where required

Co-authored-by: Peter Amstutz <peter.amstutz@curii.com>
Co-authored-by: Peter Amstutz <peter.amstutz@curii.com>
Co-authored-by: Peter Amstutz <peter.amstutz@curii.com>
* Update basic-concepts.md

* Update command-line-tool.md
Copy link
Member Author

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

My initial review.

It would be nice if more of this was caught during the original PRs, @tetron

Also, the commit messages are very uninformative. Either PRs should be squashed and the commit message updated, or contributors should be asked to clean up their commits.

src/faq.md Show resolved Hide resolved
src/faq.md Show resolved Hide resolved
src/faq.md Outdated Show resolved Hide resolved
src/faq.md Outdated Show resolved Hide resolved
src/faq.md Show resolved Hide resolved
Comment on lines +70 to +71
If a command-line tool is written manually in CWL v1.1+, there is a need to
specify when network access is required.
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
If a command-line tool is written manually in CWL v1.1+, there is a need to
specify when network access is required.
Starting with CWL v1.1, you must document the requirement for network access if it is required.


```{note}
CWL v1.0 command-line tools that are upgraded to v1.1
or v1.2 get Network Access automatically.
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
or v1.2 get Network Access automatically.
or v1.2 get `networkAccess: true` set automatically.



## Network Access
This indicates whether a process requires outgoing IPv4/IPv6 network access.
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the placement of this new section seems awkward. None of the other CommandLineTool specific requirements are documented in this section.

I think a new section about network access is fine. We should also explain why network access using steps should be avoided: hurts portability, increases the fragility of the workflow (external service can go away, or change format of responses). We should also present alternatives like putting the URL for an input File in the input object's location field instead of a curl or wget using CommandLineTool; and in general separating the automated batch steps from data acquisition and data publishing steps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add this now and then make a ticket to add this information. The ticket just said to add this specifically. #249

Copy link
Contributor

Choose a reason for hiding this comment

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

I think probably CommandLineTools are mentioned when they pop up in the context of using them - but I am not sure where this one might pop up in the context of the user guide. The possible options are (1) Keep it where it is for now (2) Add it here -- http://www.commonwl.org/user_guide/introduction/basic-concepts.html#processes-and-requirements or (3) Add it to the FAQ

Comment on lines +29 to 31
If you are using Windows, you will have to install the [Windows Subsystem for Linux 2](https://learn.microsoft.com/en-us/windows/wsl/install) (WSL2).
Visit the `cwltool` [documentation](https://github.com/common-workflow-language/cwltool/blob/main/README.rst#ms-windows-users)
for details on installing WSL2.
Copy link
Member Author

Choose a reason for hiding this comment

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

Best not to link directly to the WSL2 install docs, as users will click that first and not follow all of the linked cwltool MS Windows installation directions

Suggested change
If you are using Windows, you will have to install the [Windows Subsystem for Linux 2](https://learn.microsoft.com/en-us/windows/wsl/install) (WSL2).
Visit the `cwltool` [documentation](https://github.com/common-workflow-language/cwltool/blob/main/README.rst#ms-windows-users)
for details on installing WSL2.
If you are using Windows, you will have to install the Windows Subsystem for Linux 2 as documented in the
[`cwltool` documentation for Microsoft Windows users](https://github.com/common-workflow-language/cwltool/blob/main/README.rst#ms-windows-users).

src/introduction/prerequisites.md Outdated Show resolved Hide resolved
@swzCuroverse
Copy link
Contributor

My initial review.

It would be nice if more of this was caught during the original PRs, @tetron

Also, the commit messages are very uninformative. Either PRs should be squashed and the commit message updated, or contributors should be asked to clean up their commits.

@mr-c you point out several issues where people have including v1.0, command-line or FAQ titles that aren't questions. However that was done all ready in the user guide. So, I am not sure why @tetron was responsible for pointing them out since they are already inconsistent in the User Guide which you reviewed. I think if you want a standard or consistent use (1) you should document it (2) you should make sure it is in the User Guide correctly. Otherwise, there is no way people will know that is what you want.

Co-authored-by: LGTM Migrator <lgtm-migrator@users.noreply.github.com>
mr-c and others added 23 commits January 24, 2023 17:36
Co-authored-by: Michael Crusoe <mrc@commonwl.org>
Co-authored-by: Sarah Wait Zaranek <swz@curii.com>
* Added translation using Weblate (Portuguese)

* Added translation using Weblate (Portuguese)

* Added translation using Weblate (Portuguese)
Currently translated at 64.7% (11 of 17 strings)

Translated using Weblate (Portuguese)

Currently translated at 88.2% (15 of 17 strings)

Co-authored-by: Michael Crusoe <mrc@commonwl.org>
Translate-URL: https://hosted.weblate.org/projects/commonwl/cwl-user-guide-license/es/
Translate-URL: https://hosted.weblate.org/projects/commonwl/cwl-user-guide-license/pt/
Translation: Common Workflow Language/CWL User Guide: License
Currently translated at 3.4% (21 of 605 strings)

Translated using Weblate (Portuguese)

Currently translated at 2.1% (13 of 598 strings)

Translated using Weblate (Spanish)

Currently translated at 2.5% (15 of 598 strings)

Translated using Weblate (Spanish)

Currently translated at 100.0% (2 of 2 strings)

Translated using Weblate (Portuguese)

Currently translated at 100.0% (2 of 2 strings)

Co-authored-by: Michael Crusoe <mrc@commonwl.org>
Translate-URL: https://hosted.weblate.org/projects/commonwl/cwl-user-guide-sphinx/es/
Translate-URL: https://hosted.weblate.org/projects/commonwl/cwl-user-guide-sphinx/pt/
Translate-URL: https://hosted.weblate.org/projects/commonwl/user-guide/es/
Translate-URL: https://hosted.weblate.org/projects/commonwl/user-guide/pt/
Translation: Common Workflow Language/CWL User Guide
Translation: Common Workflow Language/CWL User Guide: Sphinx
@@ -89,11 +88,14 @@ You can run the CWL tool description by omitting the `--validate` option:
:caption: Running `true.cwl` with `cwltool`.
```

### cwl-runner Python module
### Cwl-runner Python Module
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
### Cwl-runner Python Module
### Cwl-runner Python Module
Suggested change
### Cwl-runner Python Module
### Generic ``cwl-runner`` alias

Copy link
Member

Choose a reason for hiding this comment

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

The current text may (very likely!) have been written by me 😬 The suggested text is less technical and easier to understand IMHO, so +1

@tetron
Copy link
Member

tetron commented Jan 27, 2023

@mr-c I believe branch protection on main is interfering with "Update branch" and "Commit suggestion" buttons, so I was going to close this pull request and make a new branch called staging.

@tetron tetron mentioned this pull request Jan 27, 2023
14 tasks
@tetron
Copy link
Member

tetron commented Jan 27, 2023

See #373

tetron added a commit that referenced this pull request Jan 27, 2023
@tetron tetron closed this Jan 27, 2023
@kinow kinow mentioned this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet