-
Notifications
You must be signed in to change notification settings - Fork 7
Using copilot for adding features from track/1 to 2/main #1116
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
Conversation
Adapt commit c46320b from track/1. - Add macaroon key management methods to SynapseCharm class - Integrate macaroon key handling in reconcile method - Update backup to include macaroon keys (*.key pattern) - Update tests to account for macaroon key files - Adjust coverage threshold from 89% to 87% The implementation follows the existing signing key pattern in charm.py since this branch doesn't have a separate signing_key.py module.
Adapt commit abeaed1 from track/1. - Set default_room_version to 12 in add_default_configurations - Update test to expect default_room_version field - Fix lint issue in test_backup.py with noqa comment The refactoring to consolidate default configurations in one method was already done in this branch, so only the room version change was needed.
|
New prompt for complex commit: |
- Add MAIN_UNIT_ID = 0 constant to state/charm_state.py - Convert is_main() method to @Property checking if '/0' in unit name - Add _get_unit_address() helper method for unit address formatting - Replace instance_map() with create_instance_map() using app.planned_units() - Remove set_main_unit() method and all calls from event handlers - Remove check.level from all Pebble health checks (synapse_alive, synapse_ready, nginx_ready) - Remove tests/unit/test_charm_scaling.py (extensively refactored in track/1) - Add scenario tests for multi-unit configurations with redis - Add MAS context secret fixture with proper validation - Adjust coverage threshold from 87% to 85% Main unit is now always synapse/0, workers use other unit IDs. Instance map dynamically generated from planned_units().
|
For reference, I requested Copilot to improve my prompt and this is the result: |
Add AWS_REQUEST_CHECKSUM_CALCULATION and AWS_RESPONSE_CHECKSUM_VALIDATION environment variables to S3 backup configuration with WHEN_REQUIRED value.
Replace rc_joins_remote_burst_count and rc_joins_remote_per_second configuration options with a simpler rate_limiting_level option that can be set to 'default', 'permissive', or 'unlimited'. This simplifies rate limiting configuration by providing preset levels that configure multiple rate limiting parameters at once.
DeeKay3
left a comment
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.
LGTM - small nitpick
|
Summary (generated by Copilot)
Adapt commit c46320b from track/1.
The implementation follows the existing signing key pattern in charm.py
Adapt commit abeaed1 from track/1.
The refactoring to consolidate default configurations in one method
Main unit is now always synapse/0, workers use other unit IDs.
Add AWS_REQUEST_CHECKSUM_CALCULATION and AWS_RESPONSE_CHECKSUM_VALIDATION
Replace rc_joins_remote_burst_count and rc_joins_remote_per_second This simplifies rate limiting configuration by providing preset levels
|
What this PR does
This PR aims to bring Synapse back to a single branch/track.
Since the track/1 and 2/main branches have diverged significantly over time, this is not a strict cherry-pick. Instead, it follows a “port and adapt” approach, applying the relevant changes from one branch to the other.
Due to the complexity involved, this PR intentionally excludes commits that introduce refactoring only. It focuses solely on the structural changes required to make 2/main compatible with production deployment once MAS is enabled via a flag.
Reviewers:
For simplicity and easier comparison, this PR aims to stay as close as possible to the original structure. Please consider focusing on the branch differences rather than refactoring or Python-only changes. They can be done in a further PR.
This is the prompt that I'm using:
Goal:
c46320b | Store macaroon key (#981) | OK
abeaed1 | Change default room version to 12 (#985) | OK
810fdc3 | Change main unit to always be the 0 (and more) (#870) | OK
2367b3d | Add two env vars to s3 command (#837) | OK
f6be3e1 | Add rate_limiting_level config (#990) | OK
Extra: ignore failure in Synapse Stats Exporter pebble layer.
Why we need it
Creating a single branch for Synapse.
Checklist
docs/changelog.mdwith user-relevant changesdocs/release-notes/artifacts. If no change artifact is necessary, I tagged the PR with the labelno-release-note.(e.g., in
.github/workflows/integration_tests.yaml, ensure themoduleslist is correct)terraform fmtpasses andtflintreports no errors