Adding support for S3-compatible Storage for Repository part #332

pedro-stanaka · 2020-12-04T10:24:33Z

This PR is related to issue #228. I tried to keep it simple and contained, but there were too many places where it the local filesystem was taken as a guarantee. So there was a lot of refactoring involved.

I tried to follow the conventions you used in the code, but if I failed to respect some convention please let me know and I will fix it.

Apart from the strong test suite of yours, I also tested the application inside a private hosted server with the ansible setup and it worked well for both the local and for the new S3 storage. The only problem that I could find is when migrating from local to S3, but I think in a later PR someone (or me) can provide a proper command to migrate files from local to S3 storage.

Once again, if you see any problems on the PR, let me know and I will fix in a manageable time, I spend almost a month working on this, so I would like to see it merged 😸 .

Thanks for the awesome tool you guys are providing for the community.

codecov · 2020-12-04T10:26:11Z

Codecov Report

Merging #332 (d0cb4a3) into master (3373813) will decrease coverage by 0.17%.
The diff coverage is 94.65%.

@@             Coverage Diff              @@
##             master     #332      +/-   ##
============================================
- Coverage     99.53%   99.35%   -0.18%     
- Complexity     1822     1855      +33     
============================================
  Files           295      297       +2     
  Lines          5754     5858     +104     
============================================
+ Hits           5727     5820      +93     
- Misses           27       38      +11

Impacted Files	Coverage Δ	Complexity Δ
src/Service/ReadmeExtractor.php	`96.00% <80.00%> (-4.00%)`	`7.00 <0.00> (+1.00)`	⬇️
src/Service/Organization/PackageManager.php	`92.98% <86.20%> (-7.02%)`	`19.00 <5.00> (+2.00)`	⬇️
src/Service/Dist/Storage/StorageImpl.php	`89.09% <89.09%> (ø)`	`17.00 <17.00> (?)`
src/Controller/RepoController.php	`98.57% <91.66%> (-1.43%)`	`16.00 <2.00> (+1.00)`	⬇️
src/Command/ClearMetadataCacheCommand.php	`94.73% <92.85%> (-5.27%)`	`5.00 <1.00> (ø)`
src/Controller/ProxyController.php	`100.00% <100.00%> (ø)`	`13.00 <0.00> (ø)`
src/Service/AmazonWebServices/S3AdapterFactory.php	`100.00% <100.00%> (ø)`	`8.00 <8.00> (?)`
src/Service/Dist/FilePatternFilterIterator.php	`100.00% <100.00%> (ø)`	`8.00 <8.00> (?)`
src/Service/Proxy.php	`96.83% <100.00%> (+0.26%)`	`60.00 <0.00> (+3.00)`
src/Service/Proxy/DistFile.php	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3373813...d0cb4a3. Read the comment docs.

src/Service/ReadmeExtractor.php

pedro-stanaka

Last few comments from my side.

src/Service/Dist/Storage/StorageImpl.php

ansible/roles/app.install/tasks/main.yml

akondas

Ffirst of all, many thanks for the effort 👏 🍻 .
We are happy to merge these changes.

But first, please make these minor corrections. Then we will test these changes on our side.

If everything works, we will merge next week 🎉

.env.docker

src/Service/AmazonWebServices/S3AdapterFactory.php

src/Service/Dist/Storage/StorageImpl.php

src/Service/Organization/PackageManager.php

akondas · 2020-12-04T14:16:50Z

I must admit that you create very elegant commit descriptions. I appreciate 👏

akondas · 2020-12-07T08:05:49Z

After initial testing I found two small bugs, I push a fix.
However, the scanning of packages still does not work. Look like Storage::filename return wrong path, before this PR it looks like this:

public function filename(Dist $dist): string
    {
        return sprintf(
            '%s/%s/dist/%s/%s_%s.%s',
            $this->distsDir,
            $dist->repo(),
            $dist->package(),
            $dist->version(),
            $dist->ref(),
            $dist->format()
        );
    }

As you can see, full relative path from content root is returned.

src/Service/Organization/PackageManager.php

pedro-stanaka · 2020-12-08T08:17:25Z

After initial testing I found two small bugs, I push a fix.
However, the scanning of packages still does not work. Look like Storage::filename return wrong path, before this PR it looks like this:
public function filename(Dist $dist): string
    {
        return sprintf(
            '%s/%s/dist/%s/%s_%s.%s',
            $this->distsDir,
            $dist->repo(),
            $dist->package(),
            $dist->version(),
            $dist->ref(),
            $dist->format()
        );
    }
As you can see, full relative path from content root is returned.

Now we are passing the $this->distDir as base for the filesystem so we shouldn't need this part anymore. I will take a look this afternoon and add more tests if needed. Same for the Filesystem::put() problem.

pedro-stanaka · 2020-12-09T08:04:55Z

Regarding: #332 (comment)

How did you manage to reproduce the bug? Can you help me by pointing a class or giving me the exception stack trace so I can try to write a test case?

akondas · 2020-12-09T08:12:20Z

Regarding: #332 (comment)

How did you manage to reproduce the bug? Can you help me by pointing a class or giving me the exception stack trace so I can try to write a test case?

I have a package added earlier and I run security scanner for it. I think there will be the same problem for the newly added package. Security scanner probably wasn't tested externally, only as a separate service and that's why this problem.

pedro-stanaka · 2020-12-18T07:48:43Z

Hi @akondas sorry for the immense delay in my response. I tried to test locally and now I am getting one error while doing the security check, is this by any chance the same error as you are getting?

If so, can you tell me why it does not get logged neither reported to sentry? And maybe how can I see trace and debug it?

pedro-stanaka · 2020-12-18T07:50:49Z

AH! I think I got it. I will fix it ASAP. 😃 I found one place where I didnt change how we are reading the Zip files. But still, I was hoping this exception was going to be reported to Sentry.

akondas · 2020-12-18T08:01:38Z

AH! I think I got it. I will fix it ASAP. I found one place where I didnt change how we are reading the Zip files. But still, I was hoping this exception was going to be reported to Sentry.

Yes, in prod env it will be reported to sentry 😉

pedro-stanaka · 2020-12-18T09:31:02Z

Okay, I managed to solve this SecurityScanner problem. I will use the application a bit more locally to be sure I dont find any other problems, but I guess it is fine.

pedro-stanaka · 2020-12-18T12:35:40Z

@akondas now I only have a problem with this conflict, if you could help me resolve it would be nice. I tried to resolve locally by doing a rebase, but had to run composer update --lock -W which upgraded many dependencies and ended up totally breaking the tests (and I suppose the application itself).

Detail: I am using composer v2, from what I see in your docker image you are using it as well, but I dunno if that should be a problem.

akondas · 2020-12-18T12:37:53Z

Yes, I can help you with this. I will correct it immediately when testing this branch

…ge interface Had to do many changes regarding file handling since, many places are dependent of a local file system for things to work, i.e. ReadmeExtractor.

karniv00l · 2020-12-21T12:05:11Z

tests/Doubles/FakeDownloader.php

    {
+        $basePath = $basePath ?? dirname(__DIR__, 1).DIRECTORY_SEPARATOR.'Resources';


what's the reason for level 1 in dirname and use of DIRECTORY_SEPARATOR?

Was just to avoid the relative paths with ../ on it. I had problems when using those in conjunction with the composer repositories, specially the paths. Regarding the DIRECTORY_SEPARATOR, I try to use it whenever I can for portability. If you guys prefer just / I can replace it as well.

Now running the suite again with the old code I see that I don't get any errors 🤷‍♂️ . So I can easily revert it, just say the word. 😃

It's ok, it's just that 1 is a default level for the dirname function and we don't really support windows in other places 😅

OMG, I didn't even see that (default value). I will go back to the old code to avoid confusion.

There you go! Thanks for catching such fine grained detail 👍

Cheers, good job overall 🍻

…on Api/PackageControllerTest

akondas · 2020-12-23T14:17:51Z

We tested everything (for the local filesystem) and it looks like it works, we still have s3 configuration to check. Due to the fact that holidays are coming, we will probably merge this branch in the first week of January. Forgive me for taking so long, but I would like to supervise this myself. Thanks for the work and Merry Christmas.

pedro-stanaka · 2020-12-26T12:17:23Z

Wishing you a late Merry Christmas and all good for the new year!!! I actually deployed this branch to production at my company to do a test run before the holidays, if I face some problem I will also provide a patch here. Until next year! 🖖 ---- Pedro Tanaka

…

On Wed, Dec 23, 2020 at 3:18 PM Arkadiusz Kondas ***@***.***> wrote: We tested everything (for the local filesystem) and it looks like it works, we still have s3 configuration to check. Due to the fact that holidays are coming, we will probably merge this branch in the first week of January. Forgive me for taking so long, but I would like to supervise this myself. Thanks for the work and Merry Christmas. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#332 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAT5EQ2BAVFOKKQ6DPGZFDDSWH3Z5ANCNFSM4UNHXM3A> .

akondas · 2021-01-04T11:12:34Z

Hello @pedro-stanaka, I hope all is well in the new year. Tests showed that managing the private packages works flawlessly, but we had a small problem with the proxy on s3. I am attaching an error dump:

pedro-stanaka · 2021-01-04T16:37:18Z

Okay, I will fix this ASAP. I already know the problem, the file is on remote location and fstat requires the file to be on local file system.

@akondas would it be okay if we pass the file size inside the constructor for src/Service/Proxy/Metadata.php?

akondas · 2021-01-04T16:44:06Z

I guess that should be ok 👍. Too bad I thought fstat worked for stream data as well.

We were using fstat, but sadly it does not work for remote streams I dropped the usage of the function in favor of Flysystem methods and a custom function using fstat + stream_get_meta_data

…get stream size

pedro-stanaka · 2021-01-05T12:35:02Z

@akondas I think it should be fine now. I introduced a class for the Dist File for the proxy, because there we really only had the stream, so I added class to have information about the file size we are proxying. Hope solution is fine enough.

akondas · 2021-01-06T07:17:23Z

src/Service/Proxy/DistFile.php

@@ -2,7 +2,7 @@
 /**
 * CRM PROJECT
 * THIS FILE IS A PART OF CRM PROJECT
- * CRM PROJECT IS PROPERTY OF Legal One GmbH
+ * CRM PROJECT IS PROPERTY OF Legal One GmbH.
 *
 * @copyright Copyright (c) 2020 Legal One GmbH (http://www.legal.one)
 */


✂️ 😉

akondas · 2021-01-06T07:22:50Z

Everything seems to be working properly now. Thanks for a great job. Please delete this one comment and we are ready to merge it 🎉

akondas

One more thing to fix: repman:metadata:clear-cache cli command. This is last place where local files are used 😉.

pedro-stanaka · 2021-01-06T16:41:23Z

I will work on solution for the cli command. The symfony filesystem component is a really nice helper, but wont work with the new Flysystem adapter. I will have devise a "by-hand" solution, by iterating the contents of the distDir. Luckily this is cronjob, so no problem taking a bit more of time.

…d using Flysystem classes

pedro-stanaka · 2021-01-06T19:32:50Z

@akondas I managed to find a solution for the file finding. I took " inspiration" from symfony filesystem lib. Please take a look and see if this is adequate. 👍

akondas

LGTM 👍 👏

akondas · 2021-01-07T13:00:53Z

@pedro-stanaka Thank you for all your hard work on this functionality 🙏 👏 🎖️

pedro-stanaka commented Dec 4, 2020

View reviewed changes

src/Service/ReadmeExtractor.php Outdated Show resolved Hide resolved

pedro-stanaka commented Dec 4, 2020

View reviewed changes

src/Service/Dist/Storage/StorageImpl.php Show resolved Hide resolved

ansible/roles/app.install/tasks/main.yml Show resolved Hide resolved

akondas reviewed Dec 4, 2020

View reviewed changes

.env.docker Outdated Show resolved Hide resolved

src/Service/AmazonWebServices/S3AdapterFactory.php Show resolved Hide resolved

src/Service/Dist/Storage/StorageImpl.php Show resolved Hide resolved

src/Service/Organization/PackageManager.php Outdated Show resolved Hide resolved

akondas reviewed Dec 7, 2020

View reviewed changes

src/Service/Organization/PackageManager.php Outdated Show resolved Hide resolved

pedro-stanaka force-pushed the feature/s3-storage branch from 998a7cd to c551ee3 Compare December 9, 2020 07:59

pedro-stanaka force-pushed the feature/s3-storage branch 4 times, most recently from 7532f0e to c551ee3 Compare December 18, 2020 07:16

pedro-stanaka and others added 9 commits December 21, 2020 08:29

Removing the Symfony Filesystem from PackageManager

22c2adf

Fixing last tests regarding broken DI

f4aafb8

Removing AtomicFile and FileStorage, unifying implementation of Stora…

5258f71

…ge interface Had to do many changes regarding file handling since, many places are dependent of a local file system for things to work, i.e. ReadmeExtractor.

Making PHPStan happy

a2dcb8b

Removing wrong header from file

34986b8

Adding connection factory for S3Client and changing .env files

be25be9

Fixing missing folder creation on ansible installation

ea8c904

Updating composer lock

1dedb17

Fixing location of service declaration for s3 related services

6f2c503

Downgrade symfony/validator to 5.1.7

dc18967

karniv00l reviewed Dec 21, 2020

View reviewed changes

karniv00l approved these changes Dec 21, 2020

View reviewed changes

Pedro Tanaka added 2 commits December 21, 2020 15:35

Pushing date formatting closer to assertions to avoid test flakyness …

5dd6213

…on Api/PackageControllerTest

Passing $baseDir with relative path on FakeDownloader

321868d

pedro-stanaka mentioned this pull request Dec 29, 2020

Improve performance of file operations by introducing cached adapters for Flysystem #359

Closed

Pedro Tanaka added 5 commits January 5, 2021 09:47

Fixing error while trying to get remote stream size

a2aa853

We were using fstat, but sadly it does not work for remote streams I dropped the usage of the function in favor of Flysystem methods and a custom function using fstat + stream_get_meta_data

Fixing phpstan offenses related to refactor of Metadata constructor

6cca182

Fixing PHP CS offenses

bf62f0b

Introducing abstraction for Proxy DistFile to avoid ugly function to …

f97b08a

…get stream size

Removing unused method from Storage class

c62f2c9

akondas reviewed Jan 6, 2021

View reviewed changes

akondas requested changes Jan 6, 2021

View reviewed changes

Removing auto-generated file header with wrong copyrights

33a91b2

Fixing clear cache metadata command by removing Symfony Filesystem an…

225fb8a

…d using Flysystem classes

akondas requested a review from karniv00l January 7, 2021 07:22

akondas approved these changes Jan 7, 2021

View reviewed changes

Add env variables examples for s3

d0cb4a3

karniv00l approved these changes Jan 7, 2021

View reviewed changes

akondas merged commit b863acd into repman-io:master Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for S3-compatible Storage for Repository part #332

Adding support for S3-compatible Storage for Repository part #332

pedro-stanaka commented Dec 4, 2020

codecov bot commented Dec 4, 2020 •

edited

Loading

pedro-stanaka left a comment

akondas left a comment

akondas commented Dec 4, 2020

akondas commented Dec 7, 2020

pedro-stanaka commented Dec 8, 2020

pedro-stanaka commented Dec 9, 2020

akondas commented Dec 9, 2020

pedro-stanaka commented Dec 18, 2020 •

edited

Loading

pedro-stanaka commented Dec 18, 2020

akondas commented Dec 18, 2020

pedro-stanaka commented Dec 18, 2020 •

edited

Loading

pedro-stanaka commented Dec 18, 2020 •

edited

Loading

akondas commented Dec 18, 2020

karniv00l Dec 21, 2020 •

edited

Loading

pedro-stanaka Dec 21, 2020

karniv00l Dec 21, 2020

pedro-stanaka Dec 21, 2020

pedro-stanaka Dec 21, 2020

karniv00l Dec 21, 2020

akondas commented Dec 23, 2020

pedro-stanaka commented Dec 26, 2020 via email

akondas commented Jan 4, 2021

pedro-stanaka commented Jan 4, 2021 •

edited

Loading

akondas commented Jan 4, 2021

pedro-stanaka commented Jan 5, 2021

akondas Jan 6, 2021

akondas commented Jan 6, 2021

akondas left a comment

pedro-stanaka commented Jan 6, 2021

pedro-stanaka commented Jan 6, 2021

akondas left a comment

akondas commented Jan 7, 2021

		{
		$basePath = $basePath ?? dirname(__DIR__, 1).DIRECTORY_SEPARATOR.'Resources';

Adding support for S3-compatible Storage for Repository part #332

Adding support for S3-compatible Storage for Repository part #332

Conversation

pedro-stanaka commented Dec 4, 2020

codecov bot commented Dec 4, 2020 • edited Loading

Codecov Report

pedro-stanaka left a comment

Choose a reason for hiding this comment

akondas left a comment

Choose a reason for hiding this comment

akondas commented Dec 4, 2020

akondas commented Dec 7, 2020

pedro-stanaka commented Dec 8, 2020

pedro-stanaka commented Dec 9, 2020

akondas commented Dec 9, 2020

pedro-stanaka commented Dec 18, 2020 • edited Loading

pedro-stanaka commented Dec 18, 2020

akondas commented Dec 18, 2020

pedro-stanaka commented Dec 18, 2020 • edited Loading

pedro-stanaka commented Dec 18, 2020 • edited Loading

akondas commented Dec 18, 2020

karniv00l Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

pedro-stanaka Dec 21, 2020

Choose a reason for hiding this comment

karniv00l Dec 21, 2020

Choose a reason for hiding this comment

pedro-stanaka Dec 21, 2020

Choose a reason for hiding this comment

pedro-stanaka Dec 21, 2020

Choose a reason for hiding this comment

karniv00l Dec 21, 2020

Choose a reason for hiding this comment

akondas commented Dec 23, 2020

pedro-stanaka commented Dec 26, 2020 via email

akondas commented Jan 4, 2021

pedro-stanaka commented Jan 4, 2021 • edited Loading

akondas commented Jan 4, 2021

pedro-stanaka commented Jan 5, 2021

akondas Jan 6, 2021

Choose a reason for hiding this comment

akondas commented Jan 6, 2021

akondas left a comment

Choose a reason for hiding this comment

pedro-stanaka commented Jan 6, 2021

pedro-stanaka commented Jan 6, 2021

akondas left a comment

Choose a reason for hiding this comment

akondas commented Jan 7, 2021

codecov bot commented Dec 4, 2020 •

edited

Loading

pedro-stanaka commented Dec 18, 2020 •

edited

Loading

pedro-stanaka commented Dec 18, 2020 •

edited

Loading

pedro-stanaka commented Dec 18, 2020 •

edited

Loading

karniv00l Dec 21, 2020 •

edited

Loading

pedro-stanaka commented Jan 4, 2021 •

edited

Loading