Skip to content

Prepare for Renovate with reusable workflows #239

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

Merged
merged 10 commits into from
Mar 23, 2022
Merged

Prepare for Renovate with reusable workflows #239

merged 10 commits into from
Mar 23, 2022

Conversation

ghostwriter
Copy link
Contributor

Q A
QA yes

Description

Adding Renovate to Laminas, Mezzio repositories. https://gist.github.com/weierophinney/49ca8ae2b916d898dd3d4162b26ff41b

  • Configure minimum supported PHP version
  • Update composer dependencies
  • Update to reusable Continuous Integration workflow
  • Update to reusable Automatic Releases workflow

@Ocramius Ocramius added this to the 2.14.0 milestone Feb 21, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Generated composer.lock seems to be invalid - needs to be re-generated (or rolled back to just hash update)

@ghostwriter
Copy link
Contributor Author

Unable to locate package php8.1-sqlsrv

uses: laminas/laminas-continuous-integration-action@v1
with:
job: ${{ matrix.job }}
services:
Copy link
Member

Choose a reason for hiding this comment

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

These services should probably still run

@Ocramius
Copy link
Member

Looks like checks didn't run, perhaps due to YAML structure/parsing error?

@ghostwriter
Copy link
Contributor Author

@Ocramius apparently, the services keyword is not supported yet for reusable workflows.

https://docs.github.com/en/actions/using-workflows/reusing-workflows#supported-keywords-for-jobs-that-call-a-reusable-workflow

@Ocramius
Copy link
Member

Ooof, guess we'd keep our phpunit tasks written down manually here, while excluding them from the laminas-ci integration perhaps?

Signed-off-by: Nathanael Esayeas <[email protected]>
Signed-off-by: Nathanael Esayeas <[email protected]>
@ghostwriter ghostwriter requested a review from Ocramius February 23, 2022 15:08
@ghostwriter
Copy link
Contributor Author

Removed reusable Continuous Integration workflow for now.

.laminas-ci.json Outdated
"name": "PHPUnit on PHP 8.1 with latest dependencies"
},
{
"name": "PHPUnit on PHP 8.1 with lowest dependencies"
Copy link
Member

Choose a reason for hiding this comment

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

This really shouldn't be excluded :|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR would fail otherwise, PHP 8.1 is not supported yet.

The "ignore_php_platform_requirements": {"8.1": true} syntax is not working.

PHP 8.1 Tests are still generated https://github.com/laminas/laminas-db/actions/runs/1887918057

Copy link
Member

Choose a reason for hiding this comment

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

We do though 🤔

"php": "^7.3 || ~8.0.0 || ~8.1.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant PHP 8.1 CI matrix: Unable to locate package php8.1-sqlsrv

Because it can't find the packing, it automatically fails early, before reaching the steps where it uses the services defined for the container.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm wondering if we can perhaps just skip sqlsrv tests for 8.1, rather than the whole 8.1 pipeline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but in laminas-ci.json the extensions we define for all PHP version, cause the script to fail on PHP 8.1 because one or more of the extensions doesn’t exist.

"extensions": [
        "pdo-mysql",
        "pdo-pgsql",
        "pdo-sqlite",
        "mysqli",
        "pgsql",
        "sqlite3",
        "sqlsrv"
    ],

if we remove this then we will lose those extensions on all PHP versions.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we need to update the laminas-continuous-integration-action. MS released v5.10.0 of the sqlsrv extension, and it has PHP 8.1 support. I'll add that to the CI action, and then we should be able to test 8.1 here.

Updates to laminas-continuous-integration-action should allow installation of sqlsrv extension now.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney
Copy link
Member

I wonder why no actions triggered with my last push...

@Ocramius
Copy link
Member

@weierophinney github partial outage

@Ocramius
Copy link
Member

We need to do something like pushing + squashing an empty commit here

@weierophinney
Copy link
Member

weierophinney commented Mar 23, 2022 via email

Signed-off-by: Nathanael Esayeas <[email protected]>
@weierophinney
Copy link
Member

@ghostwriter I'll figure out why the CI container isn't working, and get things sorted. Your work is done here; thanks for the patch!

Signed-off-by: Nathanael Esayeas <[email protected]>
Signed-off-by: Nathanael Esayeas <[email protected]>
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

Assuming tests pass after this run, 🚢

(Note: previous run passed, but the latest commit removes usage of a deprecated function.)

@weierophinney
Copy link
Member

@ghostwriter You overwrote a change I'd pushed to .laminas-ci.json:

    "ignore_php_platform_requirements": {
        "8.1": true
    }

That will allow deps to install in 8.1, which should allow tests to pass.

Signed-off-by: Nathanael Esayeas <[email protected]>
@weierophinney weierophinney merged commit 6f51bf5 into laminas:2.14.x Mar 23, 2022
@weierophinney
Copy link
Member

Thanks, @ghostwriter !

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

Successfully merging this pull request may close these issues.

3 participants