Closed kishore03109 closed 1 year ago
I think the middle approach ("AFTER") is the most sane approach and the one that makes the most sense. However, I was thinking if the CommitService* should be merged with the service behind it, so we maintain the current structure but enhance both GitFileSystemService and GitHubService to have the ability to commit to 2 branches for assets.
Main reason is because this process of "committing to 2 different branches" is a logic that is determined by whether the commit is for an asset or for a page/settings/navbar. We can probably enhance the commit()
function to handle specifying the repository folder to commit on, but the logic of committing to 2 different branches is isolated to asset-related functions (actually all the CRUD operations).
So in essence this is roughly the flow I imagine:
commit()
twice plus the other accompanying logic to make that happen.For reads, we already have a split between reading from page/settings/navbar vs reading media, so we will do something similar like above to display the media file from the correct location.
Just my 2 cents with the opinion of not splitting up the operations further.
Problem
This this partially solves IS-644. This is not a complete feature, but the purposes of this PR is to obtain early feedback on the approach just to achieve the
Create
operation for both GGs and non GGs flow. This should not have any impact on any non-whitelisted amp reduction repos for both GGs AND non GGs flow. This would have to be tested manually for now.Tests are failing in this PR, will be fixed in upcoming PRs, and will hold off merging this into main line until upstream tests-fixing PR is approved.
Solution
(copy pasting from RFC) This solution borrows a similar solution to what the repo service solved. We had to introduce a new drop in replacement class we didn’t wish to change most of the APIs. In addition about
Currently, to cater for GGs and preserve APIs, there exists a
RepoService
that acts as a drop in replacement. However,GitFileService
andGithubService
are not polymorphic, since they fundamentally work slightly differently from one another. Because they work differently with differing APIs, having twoCommitService
per (GitFileService vs GithubService) is chosen.I did toy with the idea of alternative B, but this did not work as well as I thought it would due to the differing APIs, which would have led to quite a bit of
if-elses
which did not seem ideal. Opting to split up the classes instead to KISS.There is also a need to store the repos at two different places, see here for more info.
Breaking Changes
Features:
All
create
actions should be committed to staging branch. Allcreate
actions on non-assets should be committed tostaging-lite
branch.Tests
Note: To the user, there should not be any difference between the GGs flow and the non-GGs flow. The key thing to note is that this particular Create operation on a non-asset change is lead to an update of both the
staging
and thestaging-lite
branch.GGs flow, with quickie feature flagged
https://github.com/isomerpages/isomercms-backend/assets/42832651/86e15eaa-79a6-4f73-8d7e-843bbf8998a4
Non-GGs flow, with quickie feature flagged
https://github.com/isomerpages/isomercms-backend/assets/42832651/d1955b6e-3a1e-401d-84bb-5e19405f1a23
[Note the two flows below are just to show that sites that are non feature flagged should be BAU, and not impact to their flow/staging branches]
GGs flow, which quickie not feature flagged
https://github.com/isomerpages/isomercms-backend/assets/42832651/157e8c45-39a2-4ccf-8344-f536d90681be
Non-GGs flow, with quike not feature flagged
https://github.com/isomerpages/isomercms-backend/assets/42832651/fa53cd8a-0e78-46a7-b664-1a9bd3e71924
Manual tests in staging
kishore-test
and notice changes in staging and staging lite.New environment variables:
env var
: env var detailsmodification of env vars
/efs/repos/
to/efs/