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

Update the GitHub Actions workflow for the Fluent linter #17198

Merged
merged 3 commits into from
Oct 28, 2023

Conversation

timvandermeij
Copy link
Contributor

The commit messages contain more details about the individual changes.

This commit tweaks the Fluent linter workflow to match the other
workflow files we have, so we make sure the steps have a newline between
them for better readability and align names and descriptions of steps
with how they are called in the other workflow files we have.
The current stable version of Python is Python 3.12, see
https://www.python.org/downloads, so we should switch to that since
Python 3.10 is older and only receives security updates.
…ent linter

I noticed the following warning in the GitHub Actions workflow logs:

`Configuration file not found: .github/linter_config.yml`

The configuration file is called `fluent_linter_config.yml` instead, so
this commit fixes the path so it points to the correct file.

Fixes 487816b.
@timvandermeij timvandermeij changed the title Improve consistency and use Python 3.12 in the GitHub Actions workflow for the Fluent linter Update the GitHub Actions workflow for the Fluent linter Oct 28, 2023
@Snuffleupagus
Copy link
Collaborator

Thanks for doing this clean-up, since consistency is always nice!

Looking at this a bit closer, I've got two questions about potential additional improvements:

  • Would it be technically possible to "combine" the push and pull_request configurations, given what we do in another workflow, since we're currently duplicating the same configuration in both?
    push:
    paths:
    - 'l10n/en-US/**.ftl'
    - '.github/fluent_linter_config.yml'
    - '.github/workflows/fluent_linter.yml'
    branches:
    - master
    pull_request:
    paths:
    - 'l10n/en-US/**.ftl'
    - '.github/fluent_linter_config.yml'
    - '.github/workflows/fluent_linter.yml'
    branches:
    - master
  • Is it correct and/or meaningful to leave the following entry blank, or can the line be removed?
    workflow_dispatch:

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Oct 28, 2023

Would it be technically possible to "combine" the push and pull_request configurations, given what we do in another workflow, since we're currently duplicating the same configuration in both?

Sadly that's not possible in this case. In that workflow we trigger on the push and pull_request events using the default configuration, so only in that case can they be combined. If we need to override the default configuration, we have to do so per event type because the actual configuration can differ between the various event types. You can find more information at https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#using-activity-types-and-filters-with-multiple-events.

Is it correct and/or meaningful to leave the following entry blank, or can the line be removed?

This puzzled me as well for a bit, but I found out that this actually has a purpose. What is does is allow this workflow to be ran manually over at https://github.com/mozilla/pdf.js/actions/workflows/fluent_linter.yml (note the "This workflow has a workflow_dispatch event trigger" banner and the "Run workflow" button). This allows running the workflow on-demand outside of a pull request, which is mostly a convenience feature. The other workflows don't have it because conceptually they make less sense to run outside of the scope of a PR (although technically they could get this too).

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, thank you!

@timvandermeij timvandermeij merged commit 973cbb0 into mozilla:master Oct 28, 2023
4 checks passed
@timvandermeij timvandermeij deleted the python-3.12-lint branch October 28, 2023 18:48
@flodolo
Copy link
Contributor

flodolo commented Oct 30, 2023

The linter is not tested with Python 3.12 🤔 I suggest to stick with 3.10 or 3.11

For me, with 3.12 tests fail to even install dependencies
https://github.com/mozilla-l10n/moz-fluent-linter/actions/runs/6694472871/job/18187850055?pr=14

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Nov 4, 2023

That particular build fails because it's fetching PyYAML 6.0 which doesn't have Python 3.12 wheels; see https://pypi.org/project/PyYAML/6.0/#files and see the PyYAML-6.0.tar.gz fetching part in those logs, which means the source package is fetched that then has to be built while the required build requirements are missing. However, in our build PyYAML 6.0.1 is fetched which fortunately does have Python 3.12 wheels and works just fine; see https://pypi.org/project/PyYAML/6.0.1/#files for the cp312 wheels and https://github.com/mozilla/pdf.js/actions/runs/6678649246/job/18149912927 for our workflow run where the "Install Fluent dependencies" picks up PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.

The difference between the builds is that we pip install the package directly so the version ranges from setup.py apply and therefore the latest PyYAML is fetched whereas the build you linked uses pip install -r .github/requirements.txt which pins PyYAML to a specific version for which no Python 3.12 wheels exists, thus building it from source.

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

Successfully merging this pull request may close these issues.

3 participants