Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[workflows] reorg #1102

Merged
merged 4 commits into from
Feb 25, 2025
Merged

[workflows] reorg #1102

merged 4 commits into from
Feb 25, 2025

Conversation

kwk
Copy link
Collaborator

@kwk kwk commented Feb 24, 2025

  • The python-format-and-tests.yml workflow did too many jobs in one workflow and didn't allow us to treat each job it did with different care.

  • We want code to be formatted with black and we want this to be mandatory in code reviews. Let's do this in check-python-code-formatting-with-black.yml from now on.

  • We want python tests to pass, so let's do this in check-python-code-with-pytest.yml.

  • We want to generate coverage but we potentially don't want to make this mandatory for pull requests. For example, see
    image
    for this PR which doesn't change anything regarding coverage numbers. See check-python-code-coverage.yml.

  • The retest.yml just got a new name: retest-chroots-on-testing-farm.yml.

  • The Makefile got dedicated target we can use from within the workflows.

In order to unblock this PR I first have to get approval for it and then I can tune the branch protection settings to no longer require these old workflows image but the new ones

image

* The `python-format-and-tests.yml` workflow did too many jobs in one
  workflow and didn't allow us to treat each job it did with different
  care.

* We want code to be formatted with `black` and we want this to be
  mandatory in code reviews. Let's do this in
  `check-python-code-formatting-with-black.yml` from now on.

* We want python tests to pass, so let's do this in
  `check-python-code-with-pytest.yml`.

* We want to generate coverage but we potentially don't want to make this
  mandatory for pull requests. See `check-python-code-coverage.yml`.

* The `retest.yml` just got a new name.

* The `Makefile` got dedicated target we can use from within the
  workflows.
@kwk kwk requested a review from tuliom February 24, 2025 10:45
Copy link
Collaborator

@tuliom tuliom left a comment

Choose a reason for hiding this comment

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

We want code to be formatted with black and we want this to be mandatory in code reviews.

I don't remember ever discussing this part with the rest of the team. IMHO, it's a good idea to have a tool that helps to check code formatting, but I've seen this test fail with super-strict results, e.g. complaining about how the lines of imports should be organized/grouped together.

Is it possible to reduce that requirement?

@tuliom
Copy link
Collaborator

tuliom commented Feb 24, 2025

There are 2 tests pending (run-tests and check-python-with-black).
Does this PR need to be merged before these tests are executed?

@kwk
Copy link
Collaborator Author

kwk commented Feb 24, 2025

We want code to be formatted with black and we want this to be mandatory in code reviews.

I don't remember ever discussing this part with the rest of the team. IMHO, it's a good idea to have a tool that helps to check code formatting, but I've seen this test fail with super-strict results, e.g. complaining about how the lines of imports should be organized/grouped together.

Actually, the pre-commit git hook which should run on every git commit locally will complain about unordered imports etc. and reformat it to match the expectation. I've introduced black probably years ago to not have discussions about any formatting whatsoever.

Is it possible to reduce that requirement?

IMHO. No because I don't see why not to follow the idea that black has. I treat it as a replacement for what the go community has with their formatting.

@kwk
Copy link
Collaborator Author

kwk commented Feb 24, 2025

There are 2 tests pending (run-tests and check-python-with-black). Does this PR need to be merged before these tests are executed?

The two pending tests are the ones, that I've split up in this PR:

  1. check-python-with-black is now here.
  2. run-tests is now here and here.

When this PR gets approved I'm going to adjust the branch protection rule to remove the two tests and instead add the pytest run and black formatting individually. At least that was the plan.

Copy link
Collaborator

@tuliom tuliom left a comment

Choose a reason for hiding this comment

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

While I disagree about the code style and I believe it should be relaxed, I have to recognize this PR is not changing the current behavior.
So, LGTM.

@kwk kwk merged commit c06a8b0 into fedora-llvm-team:main Feb 25, 2025
6 of 7 checks passed
@kwk kwk deleted the workflow-reorg branch February 25, 2025 17:33
@kwk
Copy link
Collaborator Author

kwk commented Feb 25, 2025

The branch protection rules for main are now changed to this:

image

Unfortunately the old branch protection way of configuring things only shows the job names and not the workflow names. black and pytest are the correct ones to choose though.

In the future we might wanna look into rulesets.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants