Introduce TransferCommitment API #410

VoigtS · 2024-03-05T13:55:10Z

productionChecklist:

I updated the documentation to describe the semantical or interface changes I introduced.

This PR introduces commitment transfers from a source to a target project.
Two API's will be implemented:

start-transfer: Either marks a commitment as transferrable or splits the commitment into two parts where the provided amount will be the indicator for the transferrable commitment
transfer-commitment: Where the actual commitment transfer from a source to a target AZ_RESOURCE_ID will be executed.

coveralls · 2024-03-05T14:00:27Z

coverage: 80.818% (-0.5%) from 81.277%
when pulling 7765102 on transfer
into 256a835 on master.

docs/users/api-spec-resources.md

internal/api/commitment.go

internal/api/commitment_test.go

internal/api/utils.go

majewsky

Please do not be disheartened by the number of remarks. You did a good job getting accustomed to the existing code structure and following house conventions. But there is some work left to do, both in the stylistic and functional departments:

docs/users/api-spec-resources.md

internal/api/api_test.go

internal/api/commitment.go

internal/api/commitment_test.go

internal/api/core.go

internal/api/utils.go

…nd production

Co-authored-by: Sandro <sandro.jaeckel@sap.com>

Documentation: Fix grammatical indistinctness. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Nounify variable names of SQL queries. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Token check: Remove error return message. Already handled by token.Require function. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Rename: splitCommitment function to buildSplitCommitment. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Commitment test: Update parameter from snake case to camel case. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Tests: Negative tests now check the body content

Simplify commitment transfer. Utilize ORM. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Update transfer commitment test with split commitment

VoigtS · 2024-03-15T15:43:14Z

Reminder: Check on Monday - Previous OLTP implementation had a bug in the transferred commitment.
Only the return value contained the Transferstatus and Token of the splitted commitment transferCommitment.
However, the values were not mirrored in the database.

The ORM implementation now takes this into account and saves the values in the database. A additional test case was added that tests the transfer of a splitted commitment.

majewsky · 2024-03-19T12:19:21Z

internal/api/commitment.go

+
+	// find commitment by transfer_token
+	var dbCommitment db.ProjectCommitment
+	err := p.DB.SelectOne(&dbCommitment, getCommitmentWithMatchingTransferTokenQuery, transferToken)


Should also match on ID, as defense in depth.

The query now respects the commitment ID (read out of the URL)

majewsky · 2024-03-19T12:20:37Z

internal/api/commitment.go

+
+	// get target AZ_RESOURCE_ID
+	var targetResourceID db.ProjectAZResourceID
+	err = p.DB.QueryRow(findTargetAZResourceIDBySourceIDQuery, targetProject.ID, dbCommitment.AZResourceID).Scan(&targetResourceID)


This looks like it does not match on service type and resource name. How is the commitment matched to the correct service and resource on the receiving side?

The tests can plausibly work "by accident" because they all use first/capacity which is the alphabetically first service/resource.

You are absolutely correct. I fixed this with a Common Table Expression that queries the service type, resource name and AZ from the source commitment.
Additionally I changed the tests to use the service "second". In the old implementation this caused the selected resource_az_id to be from the service "first". It now points to the correct service.

Caused: on multipyle services or resources the first entry was selected and not the values the source commitments holds

…e commitment

…String anymore

VoigtS · 2024-03-19T15:41:18Z

Fixed: Movement of the transfer token from queryString into the request header.

Respects the movement of the transfer token as queryString into the header

VoigtS changed the title ~~Add Skeleton transferCommitment endpoint~~ Introduce TransferCommitment API Mar 5, 2024

VoigtS marked this pull request as ready for review March 11, 2024 11:31

VoigtS requested review from majewsky and SuperSandro2000 March 11, 2024 11:32

SuperSandro2000 reviewed Mar 11, 2024

View reviewed changes

majewsky requested changes Mar 15, 2024

View reviewed changes

VoigtS and others added 23 commits March 15, 2024 11:15

Add Skeleton transferCommitment endpoint

fe61148

Add token check

fcaab34

Commitment: Implement commitment GET from DB. Test: Implement DB insert

e3c70cf

Test: define working request/response objects

98aa497

Add event logging

51c0c8a

Generate transfer token: Implement function to be used in unit test a…

dcea033

…nd production

Add missing changes from last commit

6c43ca1

Finalize: start-transfer API

91bb534

Add transferStatus and token to splitted commitment

c8a94a8

Check: unlisted or public status and amount (greater zero)

4cc8f6c

Add sekelton for transfer-commitment API

811f866

Implement Transfer Commitment API logic

79a988f

Only allow commitment transfer on confirmed commitments

b70b3de

Fix typo in message: project -> commitment

30b6ad4

Update API documentation

1278203

Update docs/users/api-spec-resources.md

94c4816

Co-authored-by: Sandro <sandro.jaeckel@sap.com>

Update docs/users/api-spec-resources.md

f4e5423

Co-authored-by: Sandro <sandro.jaeckel@sap.com>

Update docs/users/api-spec-resources.md

810c25a

Co-authored-by: Sandro <sandro.jaeckel@sap.com>

Update docs/users/api-spec-resources.md

fe53bb5

Co-authored-by: Sandro <sandro.jaeckel@sap.com>

Update internal/api/utils.go

42b6992

Co-authored-by: Sandro <sandro.jaeckel@sap.com>

Return error if project not found

94703ba

Tests: Change time usage to clock usage

d4c3635

Add missing error handlings

ddc1bca

VoigtS force-pushed the transfer branch from 2d119c2 to ddc1bca Compare March 15, 2024 10:16

VoigtS and others added 14 commits March 15, 2024 13:26

Update docs/users/api-spec-resources.md

2f94e47

Documentation: Fix grammatical indistinctness. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Update internal/api/commitment.go

6c3fbe8

Nounify variable names of SQL queries. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Update internal/api/commitment.go

66b7e54

Token check: Remove error return message. Already handled by token.Require function. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Update internal/api/commitment.go

ccc016d

Rename: splitCommitment function to buildSplitCommitment. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Update internal/api/commitment_test.go

726f248

Commitment test: Update parameter from snake case to camel case. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Fix: Remaining Delta from the PR code suggestions

00d9ddb

Change naming: GenerateToken to GenerateTransferToken

5652337

Fix: start transfer parseTarget struct now uses explicit fields.

c777d8d

Fix: Errors now returned as text.

abf3a0f

Tests: Negative tests now check the body content

Request with amount > commitment amount will be treated as an error

7a233cc

Update internal/api/commitment.go

92de216

Simplify commitment transfer. Utilize ORM. Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>

Fix: SQL update now takes both return values into account

8af9563

Fix: remove unused sql update query

944f2e6

Fix: Remove SQL update queries and replace them with proper ORM

f49743e

Update transfer commitment test with split commitment

Fix docstring quotes

0cfc807

majewsky requested changes Mar 19, 2024

View reviewed changes

VoigtS added 3 commits March 19, 2024 15:57

Fix: Transfer Commitment did not use service or resource identifier

2553e6c

Caused: on multipyle services or resources the first entry was selected and not the values the source commitments holds

Transfer commitment: no uses commitment ID and token to get the sourc…

fa512f6

…e commitment

Transfer commitment: now uses transfer token in header - not as query…

fc307ae

…String anymore

VoigtS requested a review from majewsky March 19, 2024 15:41

Update transfer commitment documentation

7765102

Respects the movement of the transfer token as queryString into the header

majewsky approved these changes Mar 21, 2024

View reviewed changes

majewsky merged commit f00f549 into master Mar 21, 2024
7 checks passed

majewsky deleted the transfer branch March 21, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce TransferCommitment API #410

Introduce TransferCommitment API #410

VoigtS commented Mar 5, 2024 •

edited

coveralls commented Mar 5, 2024 •

edited

majewsky left a comment

VoigtS commented Mar 15, 2024

majewsky Mar 19, 2024

VoigtS Mar 19, 2024

majewsky Mar 19, 2024

VoigtS Mar 19, 2024

VoigtS commented Mar 19, 2024

Introduce TransferCommitment API #410

Introduce TransferCommitment API #410

Conversation

VoigtS commented Mar 5, 2024 • edited

coveralls commented Mar 5, 2024 • edited

majewsky left a comment

Choose a reason for hiding this comment

VoigtS commented Mar 15, 2024

majewsky Mar 19, 2024

Choose a reason for hiding this comment

VoigtS Mar 19, 2024

Choose a reason for hiding this comment

majewsky Mar 19, 2024

Choose a reason for hiding this comment

VoigtS Mar 19, 2024

Choose a reason for hiding this comment

VoigtS commented Mar 19, 2024

VoigtS commented Mar 5, 2024 •

edited

coveralls commented Mar 5, 2024 •

edited