-
-
Notifications
You must be signed in to change notification settings - Fork 187
fix: restore latest pact for branch endpoint #887
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
| add ["pacts", "provider", :provider_name, "consumer", :consumer_name, "branch", "latest"], Api::Resources::PactVersionsForBranch, {resource_name: "latest_pact_publications_for_main_branch"} | ||
| add ["pacts", "provider", :provider_name, "consumer", :consumer_name, "branch", :branch_name, "latest"], Api::Resources::PactVersionsForBranch, {resource_name: "latest_pact_publications_for_branch"} | ||
| add ["pacts", "provider", :provider_name, "consumer", :consumer_name, "branch", :branch_name], Api::Resources::PactVersionsForBranch, {resource_name: "pact_publications_for_branch"} | ||
| add ["pacts", "provider", :provider_name, "consumer", :consumer_name, "branch", "versions"], Api::Resources::PactVersionsForBranch, {resource_name: "pact_publications_for_main_branch"} |
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.
Will this endpoint never be accessible, because the trailing versions static path item might match the dynamic :branch_name path parameter on line 36?
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.
yep, good catch, added some tests in to cover this, and the initial bug of overwriting the original endpoint which returned pact content
| query = self | ||
| query | ||
| branch_versions_join = { | ||
| Sequel[:branch_versions][:version_id] => Sequel[:pact_publications][:consumer_version_id], |
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.
In adding tests, I noted that I negated to add in logic to scope this to main branches 😅 added now 👍🏾
|
|
||
| it "returns the latest pact for the branch" do | ||
| expect(json_response_body[:_links][:self][:href]).to end_with("2") | ||
| expect(json_response_body[:interactions].length).to eq(1) |
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.
This will catch the original bug, by ensuring we get pact content returned from this endpoint
|
Would appreciate a re-review, have added tests to cover the newly added endpoints which exposed a discrepancy with one of the logic flows which is mopped up now. danke danke |
There was an overlap when introducing #728
which resulted in
/pacts/provider/{provider}/consumer/{consumer}/branch/{branch}/latestreturningpact-versions, not the expected latest pact content, for a named branch of an application pair.I have appended new routes with
/versionsto match existing pact_publications resource.It does not append
/versionstopact_publications_for_branchroute, as this is also used for deleting Pact versions for branch (such supporting deleting pacts by branch was introduced in the UI).