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

Fix finding composer cache dir #233

Merged
merged 4 commits into from Oct 8, 2023
Merged

Fix finding composer cache dir #233

merged 4 commits into from Oct 8, 2023

Conversation

TamasSzigeti
Copy link
Contributor

@TamasSzigeti TamasSzigeti commented Sep 30, 2022

Description

Fix missing working dir when getting composer cache dir config
Bumping included composer.phar version to fix CI error – to the minimum that has this fix

Motivation and context

Fixes #225

How has this been tested?

Tested by manually running composer_paths.sh in a directory where there is no composer.lock
Current tests do not fail because there is another composer.lock in tests dir.
Added another test case, hope it works (:

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR checklist

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • I have added tests to cover my changes.

@TamasSzigeti TamasSzigeti marked this pull request as ready for review September 30, 2022 06:27
@TamasSzigeti TamasSzigeti marked this pull request as draft September 30, 2022 06:29
@TamasSzigeti TamasSzigeti marked this pull request as ready for review September 30, 2022 06:43
@TamasSzigeti
Copy link
Contributor Author

@ramsey Could you have a look, please?

@TamasSzigeti
Copy link
Contributor Author

@ramsey Fixed the (unrelated) CI error before I noticed it is also done in #237
This should be green now

@TamasSzigeti
Copy link
Contributor Author

@ramsey Fixed conflict

@TamasSzigeti
Copy link
Contributor Author

@ramsey Seems the v1 fail already happens in upstream branch, so this might be good to go?

@ramsey
Copy link
Owner

ramsey commented Nov 3, 2022

@TamasSzigeti Please update your branch with the latest upstream changes. Thanks!

@TamasSzigeti
Copy link
Contributor Author

@TamasSzigeti Please update your branch with the latest upstream changes. Thanks!

@ramsey Done

@TamasSzigeti
Copy link
Contributor Author

@ramsey Adjusted the test for the changed github output

@TamasSzigeti
Copy link
Contributor Author

@ramsey Is there anything yet to be done here?

@ramsey
Copy link
Owner

ramsey commented Dec 9, 2022

@TamasSzigeti Are you able to fix the test failures?

@TamasSzigeti
Copy link
Contributor Author

@ramsey
When it failed it was already failing in master, not coming from my changes. I rebased and will fix if it still fails

@petemcw
Copy link

petemcw commented Feb 13, 2023

Has there been any further progress on this? I can report that caching does not work correctly when using a "working-directory" input.

@mxr576
Copy link

mxr576 commented Aug 21, 2023

Meh, I had just opened a duplicate in #247 before I found this 🤦‍♂️

@mxr576
Copy link

mxr576 commented Aug 21, 2023

Related issue: #246

@TamasSzigeti
Copy link
Contributor Author

TamasSzigeti commented Aug 21, 2023

@mxr576 I totally forgot about this PR, maybe you could take it over, fix tests?

@mxr576
Copy link

mxr576 commented Aug 28, 2023

@mxr576 I totally forgot about this PR, maybe you could take it over, fix tests?

I cannot make any promises, tests were not executed on my PR because I am a first time contributor and I do not see failures due to: "The logs for this run have expired and are no longer available."

Rebasing/retriggering the build could be useful

@ramsey
Copy link
Owner

ramsey commented Aug 28, 2023

@mxr576 If you want to reopen your PR, I can click the button to allow the tests to run.

@mxr576
Copy link

mxr576 commented Aug 29, 2023

Thanks, reopened

@TamasSzigeti
Copy link
Contributor Author

TamasSzigeti commented Oct 8, 2023

@ramsey Sorry for taking this long, I fixed the tests, should be all green now.

@mxr576
Copy link

mxr576 commented Oct 8, 2023

@mxr576 I totally forgot about this PR, maybe you could take it over, fix tests?

Same here... ;S thx!!! \o/

@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Merging #233 (d68c9ba) into v2 (12e7194) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##               v2     #233   +/-   ##
=======================================
  Coverage   84.23%   84.23%           
=======================================
  Files           7        7           
  Lines         203      203           
=======================================
  Hits          171      171           
  Misses         32       32           
Files Coverage Δ
bin/composer_paths.sh 97.43% <100.00%> (ø)

@ramsey ramsey merged commit c49029a into ramsey:v2 Oct 8, 2023
59 checks passed
@ramsey
Copy link
Owner

ramsey commented Oct 8, 2023

Thank you!

@TamasSzigeti TamasSzigeti deleted the fix-composer-cache-dir branch October 9, 2023 08:23
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.

Cannot find composer.json
4 participants