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

Add workflows related to translation updates #21

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add workflows related to translation updates #21

wants to merge 3 commits into from

Conversation

nvdaes
Copy link
Contributor

@nvdaes nvdaes commented Sep 12, 2021

Background

This has been discussed on the add-ons mailing list with @abdel792

@mhameed is the author of the workflow to check for symbols like BOM, which cause problems with Gettext, and help me a lot to practice and learn GitHub Actions.
@ABuffEr, @lukaszgo1, @CyrilleB79 and @grisov (who created a workflow to perform typing checks with Mypy) maybe interested.

This pull request also includes a workflow to update translations from stable branch of add-ons hosted in this organization, copying doc and locale folders to personal repos, once every hour.
I use these workflows in my add-ons. For this pr I have included some comments for clarification.

@@ -0,0 +1,31 @@
# https://crontab.guru/crontab.5.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Placing this file in .github/workflows has an unfortunate side effect that this action would be run for addonTemplate repo which is not what we want. Perhaps these sample actions should be moved to something like GitHubActions/sampleActions and documented in the readme.

Copy link
Contributor

Choose a reason for hiding this comment

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

Disabling this action for add-on template repository would work but only for this repo. If someone uses this template as is and their add-on is not registered in the translations system they would be getting failures by default. I still maintain that moving these files from .github/workflows and documenting each of these actions in the readme is the simplest way to deal with the problem though I don't have any authority over this repository obviously.

run: |
git config --global user.name github-actions
git config --global user.email [email protected]
git remote add l10n https://github.com/nvdaaddons/addonName # Replace with the repo name, for example, clipContentsDesigner
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be nice not to hardcode name of the repository. Have you tried playing with GITHUB_REPOSITORY to get the repo name?

git pull
git push


Copy link
Contributor

Choose a reason for hiding this comment

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

A new line here please.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still not addressed though not very important obviously.

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 20, 2021 via email

@ABuffEr
Copy link

ABuffEr commented Sep 20, 2021

Placing this file in .github/workflows has an unfortunate side effect that this action would be run for addonTemplate repo which is not what we want.

Would it be possible to perform actions only if repo is not addonTemplate? As an additional solution to disable for that repo.

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 20, 2021 via email

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 21, 2021

@lukaszgo1 , I think that I've addressed your suggestions.
@ABuffEr , I'm not successful trying to disable the workflow in clipContentsDesigner add-on by checking the repo name. I think that the bestoption is to disable workflow on GitHub. Each workflow has an Options menu to select a statebadge (I don't know exactly what is this, this is a kind of image), and an option to disable/enable.

@CyrilleB79
Copy link
Contributor

I have never played with GitHub actions, so I will not review the yaml files.

However, it seems to me that a paragraph in the readme would be worth. For now the readme does not even speak about hosting the add-on on GitHub.
E.g.

To perform automatic check

If you decide to host your add-on on GitHub some auto check are available. Just copy the file xxx.yaml from GitHubActions/sampleActions to .github/workflows.

Check BOM

...

Check translation

If your add-on is registered in auto translation system, you can ...
...

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 22, 2021 via email

@CyrilleB79
Copy link
Contributor

Of course this needs to be documented in the readme as optional code to be included. Since there are other workflows for linting, testing with NVDA, releasing etc., I haven"t writen any documentation until all or some of them can be accepted.

Having the doc in the same PR allows to ease review and to have an up-to-date documentation when merged.

Where the new line should be writen? After git push?

Where is this git push? I do not know what you are speaking of.

I think that it"s easier that the yaml files are in the workflows folder and disablin them, even individually, for example if someone doesn"t register an addn to be translated, all the yaml files can be included and disabled just checkfortranslations.yaml ol, alternatively, just include the desired files.

I think that all the actions should be optional and thus disabled by default. Remember that some may use the template with add-ons not part of the translation system or even not hosted on GitHub. I agree that enabling the actions should be easy. I have no preference between:

  • moving a file from a sample folder to .github/workflows
  • renaming a file in .github/workflows from action.yaml.example to action.yaml.
    Or do you imagine a third possibility?

Technically, it is true that the doc asks to create an empty folder and to move files and folders from the template to the add-on's folder. Thus having .yaml files in the .github/workflows of the template should not be a problem for the users who do not use actions. They just do not need to copy the .github/workflows files in their add-on folder.
However, I personnaly prefer copying the whole template for my add-on's first commit and then remove the unneeded files or modify files in the subsequent commits. This allows me to maintain an addonTemplate branch and merge the updates made in the template in my personal add-on.

Note, I thought that instructions regarding linting were added to the template but this does not seem to be the case. Am I right?

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Sep 22, 2021

Where the new line should be writen? After git push?

If you look at the diff from this PR git complains that the action for fetching translations does not end with the carriage return just move to the end of file and press enter :-)

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Sep 22, 2021

Of course this needs to be documented in the readme as optional code to be included. Since there are other workflows for linting, testing with NVDA, releasing etc., I haven"t writen any documentation until all or some of them can be accepted.

Having the doc in the same PR allows to ease review and to have an up-to-date documentation when merged.

+1 to this. Also who exactly can approve or reject this PR - it is not clear who has a final say for this repository.

I think that all the actions should be optional and thus disabled by default.

I agree with this. I also tent to copy the entire template rather than picking particular files. If these actions are going to remain where they are now readme should clearly document how to disable them. Given that this procedure may change over time as GitHub modifies their interface not enabling them by default seems easier.

Note, I thought that instructions regarding linting were added to the template but this does not seem to be the case. Am I right?

What exactly should be documented regarding linting in your opinion?

@CyrilleB79
Copy link
Contributor

Note, I thought that instructions regarding linting were added to the template but this does not seem to be the case. Am I right?

What exactly should be documented regarding linting in your opinion?

Inform that this template has been linted with NVDA rules and describe how to lint an add-on based on this template.

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 23, 2021

Hello, replying afterreading all your comments:

Seems that workflows are enabled by default when someone creates a repo, even if the repo is created from a template and the workflow is disabled in the template repo.
I've tested it setting clipContentsDesigner as a template repo, in this case just for testing purposes, disabling the checkForTranslations workflow in the original repo. After that, I've created a test repo, fromclipContentsDesigner template repo. The result is that the test repo has the checForTranslations.yaml workflow enabled.

My suggestion is:

  • Add workflows to the addonTemplate repo in .github/wirkflows folder.
  • Make this a template repo (I think this is not done for now), so people can use it easily to create other repos with the same structure without cloning it before, and to indicate that this is precisely a template.
  • Disable not needed workflows in this template repo.
    • Document, especially for the checkForTranslations.yaml file, that add-ons not registered in the translations system should have this workflow disabled and instruct authors about how to do it.

Regarding linting, I use a flake8.ini and a workflow (yaml file) to lint all files inthe add-on. Previously I just linted diffs in pull requests. Authors may want to disable this too if they don't want to lint all Python files contained in the addons folder. Of course the configuration file can be set to exclude paths, useful for example if an add-on uses a third party library.

@lukaszgo1 ,the maintainer of this addonTemplate repo is @josephsl

@ABuffEr
Copy link

ABuffEr commented Sep 23, 2021

Hi @nvdaes ,
maybe it's a daredevil idea, but... is gh (github-cli) usable from a template?
And, if yes, can you instruct a template x.yaml to runs "gh workflow disable x.yaml" when execution conditions fail?
Just to increase automation...

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 23, 2021 via email

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 24, 2021

@ABuffEr and all, seems that your idea can be a reality. The problem is that now I don't know the option to enable the workflow from github.com interface. But when I try to run it from GitHub CLI, I get a message saying that it's not possible to run a disabled workflow.

https://github.com/nvdaes/test/actions/runs/1269376153/workflow

I can enable the workflow again from GitHub CLI.

@ABuffEr
Copy link

ABuffEr commented Sep 24, 2021

Hi,
well, neither I know from Github, but are you saying that it's different as usual?
Just to clarify, my idea was:

  • dev (that we can suppose a novice and/or creating a new add-on not official yet) create a new repo using this as template;
  • included workflows run automatically, so checkForTranslations.yaml verify that there aren't conditions to run (stable branch, fork on nvdaaddons, etc), and autodisable itself, without dev intervention;
  • when add-on is ready, dev asks for inclusion in translation system, and receive an ok to re-enable the checkForTranslations.yaml workflow.

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 24, 2021 via email

@nvdaes
Copy link
Contributor Author

nvdaes commented Sep 28, 2021

Í"ve been thinking and I believe that a possibility will be to store the checkForTranslations.yaml file in an external repo, not in add-on template, and, when the personal add-on repo is folforked in this nvdaaddons organization, trigger an event to copy (and merge) the checkFor translations yaml file in the personal repo. So, in the add-on template repo we may include a yaml file for this, which is run in the fork event, since we fork repos just when people request the registration in the translation system. Since this is temporal and a different system is planned, we can store the checkForTranslations workflow in the existing l10N test repo created by @mhameed and also contributed by me for testing.

@josephsl
Copy link
Contributor

Is this ready for review/merge in 2023? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants