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

Adopt Laminas CI Workflow #26

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Adopt Laminas CI Workflow #26

merged 3 commits into from
Apr 8, 2021

Conversation

Slamdunk
Copy link
Contributor

Blocks #25

Signed-off-by: Filippo Tessarotto <[email protected]>
@froschdesign
Copy link
Member

@Slamdunk
The composer.lock file must be added.

@froschdesign froschdesign added this to the 2.10.0 milestone Mar 25, 2021
@Slamdunk
Copy link
Contributor Author

I have never understood how and why: I would generate it under my local PC settings, which is arbitrary and way different from CI.

Is this ok? If so, how is it useful in the first place?

@froschdesign
Copy link
Member

Is this ok? If so, how is it useful in the first place?

See the checks running on this pull request, the CI workflow is still missing.

See also:

Signed-off-by: Filippo Tessarotto <[email protected]>
@Slamdunk
Copy link
Contributor Author

Slamdunk commented Mar 25, 2021

I've committed and pushed it. Still, I feel you didn't answer my question:

  1. https://github.com/laminas/laminas-ci-matrix-action#create-a-ci-matrix-for-use-in-a-github-action doesn't require a composer.lock committed:

    Whether to run against a "locked" set of dependencies based on the presence of a composer.lock file.

  2. Switch from Travis-CI to GitHub Actions laminas-feed#35 doesn't say how someone should generate it

@Slamdunk
Copy link
Contributor Author

Slamdunk commented Mar 25, 2021

Now build passes, PSalm fails due to laminas/laminas-continuous-integration-action#9

@froschdesign
Copy link
Member

Still, I feel you didn't answered my question…

You are absolutely right, but I am not the expert on this topic and so I can not much help here, unfortunately only with some hints.

@Slamdunk
Copy link
Contributor Author

Do you think we can close 2.10.0 milestone today?

@Slamdunk
Copy link
Contributor Author

I see you put this into 2.10.0 milestone, but there's no 2.10.x branch

@froschdesign
Copy link
Member

I see you put this into 2.10.0 milestone, but there's no 2.10.x branch

Right, I just saw that too.
I'm on the road and need to check what I can do via the GitHub app.

@Slamdunk
Copy link
Contributor Author

I'm on the road and need to check what I can do via the GitHub app.

Oh, never mind, please wait for the proper time and location to work on this 😉

@weierophinney
Copy link
Member

've committed and pushed it. Still, I feel you didn't answer my question:

  1. https://github.com/laminas/laminas-ci-matrix-action#create-a-ci-matrix-for-use-in-a-github-action doesn't require a composer.lock committed:

    Whether to run against a "locked" set of dependencies based on the presence of a composer.lock file.

It's needed for non-unit test checks. The reason is because updates to QA tooling can quickly lead to different results on different runs of the same command. As examples:

  • Things not previously flagged as CS issues now are.
  • Things not previously caught by static analysis now are, even though no relevant code changes were made.

As such, we want to commit a lockfile so that these remain idempotent between invocations on the same set of code.

  1. laminas/laminas-feed#35 doesn't say how someone should generate it

You're right, so I wrote up the process here: https://gist.github.com/weierophinney/b003e50c3c2667d08076caf31ebd36a4 (and just added the bit about Psalm cache directory issues as well).

@weierophinney
Copy link
Member

@Slamdunk Okay, the laminas-continuous-integration-action is updated to resolve the Psalm caching issue. Now the check is failing due to actual errors!

Ping me when resolved, and I'll get this merged and released.

Comment on lines +163 to +168
<RedundantCastGivenDocblockType occurrences="4">
<code>(bool) $flag</code>
<code>(string) $description</code>
<code>(string) $name</code>
<code>(string) $type</code>
</RedundantCastGivenDocblockType>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These redundant casts cannot be purged as they would result in a BC break, see:

public function testSettingDescriptionShouldCastToString(): void
{
$message = 123456;
/** @psalm-suppress InvalidScalarArgument */
$this->parameter->setDescription($message);
$test = $this->parameter->getDescription();
$this->assertNotSame($message, $test);
$this->assertEquals($message, $test);
}

Copy link
Member

Choose a reason for hiding this comment

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

You can suppress them individually, or add them to the baseline, then.

@Slamdunk
Copy link
Contributor Author

Slamdunk commented Apr 2, 2021

Ping @weierophinney

@weierophinney weierophinney modified the milestones: 2.10.0, 2.9.2 Apr 8, 2021
@weierophinney weierophinney merged commit b91fd8a into laminas:2.9.x Apr 8, 2021
@Slamdunk Slamdunk deleted the laminas_ci_workflow branch April 8, 2021 13:10
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