-
Notifications
You must be signed in to change notification settings - Fork 248
do not allow using crr location as a locationConstraint #5806
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
Hello kerkesni,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
171a8ec
to
f209c2a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.1 #5806 +/- ##
===================================================
+ Coverage 75.83% 75.88% +0.05%
===================================================
Files 188 188
Lines 11970 11970
===================================================
+ Hits 9078 9084 +6
+ Misses 2892 2886 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
23f798b
to
1bf8301
Compare
lib/Config.js
Outdated
if (locationConstraints[l].type === 'crr') { | ||
assert(locationConstraints[l].isCrr === true); | ||
} |
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.
If we always have the "type" crr and expect isCRR to be true, why not just relying on the type
to detect if this is a CRR location instead?
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 point is actually the opposite: there may be multiple "types" of locations which support CRR (i.e. must be set in backbeat replication endpoint) but cannot be used for writing directly. So the flag isCrr
was introduced to give this flexibility; and type may be used if we have different kinds of crr locations.
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/improvement/CLDSRV-653 && \
git merge origin/development/9.1 Resolve merge conflicts and commit git push origin HEAD:improvement/CLDSRV-653 |
1bf8301
to
8003aa0
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
172108f
to
d1c7b5b
Compare
d1c7b5b
to
bad4440
Compare
Replaced all instances of 'location-dmf-v1' and 'location-crr-v1' in test files with references to global constants Issue: CLDSRV-653
69cfaf5
to
53c4494
Compare
/approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-653. Goodbye kerkesni. The following options are set: approve |
Issue: CLDSRV-653