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

[CI] Test matrix: remove php7.3, mysql5.7. #886

Merged
merged 19 commits into from
Jul 27, 2022

Conversation

rosiel
Copy link
Member

@rosiel rosiel commented Jul 19, 2022

GitHub Issue: #885

Discussion on Slack: https://islandora.slack.com/archives/CM5PPAV28/p1658234802046979 (no replies yet)

What does this Pull Request do?

Updates the testing matrix

What's new?

  • instead of drupal 9.3.x and 9.4.x-dev, test 9.4.x and 9.5.x-dev
  • instead of testing php 7.3, 7.4, 8.0, and 8.1; only test 7.4, 8.0 and 8.1. Drupal 9.4 has dropped support for 7.3.
  • instead of allowing failure on php 8.0 and 8.1, make those mandatory.
  • instead of testing on mysql 5.7 and 8, only test on 8. (isn't our DB work handled by Drupal's DB abstraction layer anyway?)
  • remove testing Drupal 10 (breaks always)
  • Update the badge on the README to say php >= 7.4

How should this be tested?

  • Do these tests pass?
  • Are they good versions to test?
  • Is the new declaration ok? (I personally think having the badge match the testing makes sense, though i'm pretty sure our php )

Documentation Status

  • Does this change existing behaviour that's currently documented?
  • Does this change require new pages or sections of documentation?
  • Who does this need to be documented for?
  • Associated documentation pull request(s): ___ or documentation issue ___

Additional Notes:

Any additional information that you think would be helpful when reviewing this
PR.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/8-x-committers

@rosiel
Copy link
Member Author

rosiel commented Jul 20, 2022

After thought and discussion with @alxp , it's probably more useful to use tests to look forward (even if they fail, as they do, on our dependencies like Stomp isn't yet PHP 8.1 friendly and Contexts won't install on D10), rather than have the nice green checkmark in the pull request list 😢. So...

  • PHP 8.1 is in the matrix but as allowed_failure.
  • Drupal 10.0.x-dev is in the matrix but only on PHP 8.1

In order to make it easier for PR reviewers to understand the test output, we can use the Branch protection's "required tests" - they show up with a special "Required" badge, and I guess we can use that to evaluate if the PR is ready to merge.

(we'll need to change the required tests for this PR to be merged)

@rosiel
Copy link
Member Author

rosiel commented Jul 20, 2022

It might be time to update the rest of islandora's testing matrices too. I propose:

  • let's stop testing php 7.3
  • let's add php 8.0 and 8.1 where missing
  • for drupal modules, let's test D9 as 9.4.x and 9.5.x-dev (with intention to update this matrix as new versions release).

Open questions for Drupal:

  • Should we include drupal 10.0.x-dev in the matrix expecting it to fail?
  • MySQL 8 is out. Should we add it? and/or replace 5.7?
  • Should we update modules still testing on D8 to 8.9.20 (from 8.9.11)?

@rosiel
Copy link
Member Author

rosiel commented Jul 20, 2022

Discussion from the tech call:

@rosiel
Copy link
Member Author

rosiel commented Jul 21, 2022

I finally went with my conscience, and removed php 8.1 and drupal 10 since we haven't had a chance to actually address them yet, and made issues about what we know so far. This way,

  • we don't have jobs in the PR checks falsely marked as passing (it takes a lot of digging from the PR screen to see errors)
  • we don't have the PRs and the CI badge marked as failing. I think that's too discouraging to be useful.

@rosiel rosiel changed the title Test matrix: remove php7.3, mysql5.7. [CI] Test matrix: remove php7.3, mysql5.7. Jul 27, 2022
@rosiel rosiel requested a review from seth-shaw-asu July 27, 2022 18:04
@seth-shaw-asu seth-shaw-asu merged commit 3c194cc into Islandora:2.x Jul 27, 2022
@rosiel rosiel deleted the no-drupal-10 branch November 25, 2022 20:48
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.

2 participants