-
-
Notifications
You must be signed in to change notification settings - Fork 278
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
Add display placeholder for EAA edits in production #3271
Conversation
63c2c27
to
674eb22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The code looks good. I also checked the diffs to the branch
mwiencek/event-art-archive
. - But the commit message(s)/pull request description(s) are lacking context and references to the main pull request and ticket.
- Also the deployment plan is unclear as we might want to keep the React component
BetaOnlyEdit
and its translations in the final release, but not the rest of the code, which is either temporary or borrowed (and would be missing the context of their original commits.)
I would suggest to split into two different pull requests:
The 1st PR:
- would contain the added file
root/edit/details/BetaOnlyEdit.js
only, - would target the branch
production
(still), - could be merged and then immediately carried forward to the other branches for translations, since this change would neither rely on nor conflict with anything else.
The 2nd PR:
- would contain the rest of the (borrowed/temporary) code,
- would target the branch
production
(still), - could be merged only after carrying the changes from the 1st PR forward to the other branches for translations (as suggested above),
- could be reverted on final release time from the branch
production
before merging the branchbeta
intoproduction
.
For deployment, with two such pull request, I would suggest to:
- Merge the 1st PR into the branch
production
- Merge the branch
production
intomaster
- Merge the 2nd PR into the branch
production
- Build
production
images - Update
prod
containers - Merge the main PR into the branch
master
- Release
beta
(which includes updating the POT files with the source string for the React componentBetaOnlyEdit
)
On final release time:
- Revert the 2nd PR from the branch
production
- Release
production
(which includes merging the branchbeta
intoproduction
)
Thanks for checking this. Regarding the deployment, I wasn't planning to revert anything per se:
I don't see any need to revert anything in this scenario because step 4 would completely supplant the borrowed/temporary code (while leaving the Edit: I updated the commit message and PR description to include a reference to IMG-84 and PR #3150. I cancelled both the CircleCI and Selenium test runs, since I only changed the commit message. (Both were passing earlier.) |
674eb22
to
52bcbe3
Compare
This is a prerequisite step for IMG-84 / PR metabrainz#3150. If you were to submit any of the new EAA edits on the beta server once it's deployed there, and then try to load any of those edits in production (whether directly or via an edit listing), then it would cause an ISE in production. Pulling all of the display code for these new edit types would basically mean having to merge a very large chunk of the event-art-archive branch, as they depend heavily on changes to the `Data::` and `Entity::` layers. This patch adds a placeholder component, `BetaOnlyEdit`, which can be used to link these EAA edits to the beta server (saying that they can only be viewed there temporarily). In a subsequent commit I will add in the machinery to actually make use of this component for the EAA edits specifically. It may also be useful for other new edit types we introduce in the future.
Adds constants defining all of the edit type IDs needed for IMG-84.
This is a prerequisite step for IMG-84 / PR metabrainz#3150. It adds temporary (stub) edit classes that don't do anything except display the type of edit. The core display of the edits is delegated to the `BetaOnlyEdit` component added in a8e12fd.
52bcbe3
to
e5d0ff4
Compare
One of the motivations was:
Won’t step 4 remove the borrowed code from the original commits in the main pull request? After the final release, won’t |
That sounds right, although I also split the commits here so that they'd point to a commit specifically introducing the stub edit classes. Does that seem okay? If you'd prefer reverting so that the files are removed and re-added, I'm okay with that too, as having split the commits should make that easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for having split into 3 commits with proper messages, no need for a separate PR indeed. Yes, I would still prefer reverting the 2 stub commits in master
rather than cutting a part of the original commits in the main pull request.
OK, I will try to follow the detailed deployment process you outlined. 👍 |
Problem
This is a prerequisite step for IMG-84 / PR #3150.
If you were to submit any of the new EAA edits on the beta server once it's deployed there, and then try to load any of those edits in production (whether directly or via an edit listing), then it would cause an ISE in production.
Pulling all of the display code for these new edit types would basically mean having to merge a very large chunk of the event-art-archive branch, as they depend heavily on changes to the
Data::
andEntity::
layers.Solution
This patch just adds a placeholder component,
BetaOnlyEdit
, which links to the edit on the beta server (saying that it can only be viewed there temporarily).Testing
Tested manually on my local development server.