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

Support only 5.5 to 5.8 #112

Merged
merged 6 commits into from
Oct 23, 2020

Conversation

dstepe
Copy link
Contributor

@dstepe dstepe commented Oct 22, 2020

I reduced support on this branch Laravel 5.5 - 5.8. Anyone wanting Laravel 6 or higher should use the 3.x branch.

I added a really basic test of the ServiceProvider to validate the method of fetching the log. That test can be expended to cover more behaviors of the ServiceProvider, but I suggest doing that on the 3.x branch and back-porting to here if we want.

I want to do some more real-world testing before this is merged, even though the tests are passing.

@arkaitzgarro
Copy link
Owner

You can remove the ScheduledTaskCollector class and the RecordTransaction middleware for jobs, as those are L6 specific.

@dstepe dstepe marked this pull request as ready for review October 23, 2020 00:44
@dstepe
Copy link
Contributor Author

dstepe commented Oct 23, 2020

Closes #94

I think this is ready. The tests are passing on Laravel versions 5.5 through 5.8, including a new test of the ServiceProvider to make sure getting the Monolog instance works.

I'm not sure why the GitHub actions do not appear on this PR. They are running in my fork and have passed, though.

Copy link
Owner

@arkaitzgarro arkaitzgarro left a comment

Choose a reason for hiding this comment

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

I added the event to run GH actions on PRs, but those are failing here and not in your repo... I don't really get it.

Re-running the jobs solved the issue, computers...

.github/workflows/run-tests.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
@dstepe
Copy link
Contributor Author

dstepe commented Oct 23, 2020

There is something very odd going on with the jobs. As you noted, they fail, but then succeed when re-run. I have observed that on more than one occasion, enough to suspect there is a real problem that needs to be addressed. Again this morning the jobs started by commit failed in your project but passed in mine. Re-running them in yours allowed them to pass.

I'm wondering if there is a cache order problem or something. I see that you are caching the composer cache directory, but not the vendor directory itself. I also see that the cache key in the unit test workflow includes the Laravel version, but the restore key does not. I don't know yet why any of that would make a difference, if it does at all. I'm just making observations at this point.

After some more reading on actions/cache@v2, the implementation make sense. I'm even more stumped now.

Copy link
Owner

@arkaitzgarro arkaitzgarro left a comment

Choose a reason for hiding this comment

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

Did you test it on real applications, to make a 2.0 release?

@arkaitzgarro
Copy link
Owner

There is something very odd going on with the jobs. As you noted, they fail, but then succeed when re-run. I have observed that on more than one occasion, enough to suspect there is a real problem that needs to be addressed. Again this morning the jobs started by commit failed in your project but passed in mine. Re-running them in yours allowed them to pass.

I'm wondering if there is a cache order problem or something. I see that you are caching the composer cache directory, but not the vendor directory itself. I also see that the cache key in the unit test workflow includes the Laravel version, but the restore key does not. I don't know yet why any of that would make a difference, if it does at all. I'm just making observations at this point.

After some more reading on actions/cache@v2, the implementation make sense. I'm even more stumped now.

It makes sense to be a caching problem, because Composer seems to install/load from cache correctly, but somehow some files are missing. If the problem persists, we can disable the cache as first measurement.

@arkaitzgarro arkaitzgarro merged commit 5960825 into arkaitzgarro:2.x Oct 23, 2020
@dstepe
Copy link
Contributor Author

dstepe commented Oct 23, 2020

@arkaitzgarro Yes, I neglected to mention, but I did run install and run each version of Laravel 5.5 through 5.8 with example app. It hits most of the major features and there were no obvious issues.

@dstepe dstepe deleted the chore/fix-up-for-5.x-support branch October 25, 2020 12:36
@arkaitzgarro arkaitzgarro mentioned this pull request Oct 27, 2020
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.

None yet

2 participants