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.

Sign up for GitHub

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

Jump to bottom

Remove Acquire #266

Merged

denopink merged 9 commits into feat/webhook-update from acquire

Nov 21, 2024

Contributor

maurafortino commented Nov 18, 2024 •

edited

Loading

What's Included:

Removing the bascule/acquire package and replacing it with ancla/acquire

TODO:
-discuss solution
-fix unit tests after solution discussion

maurafortino added 3 commits

November 18, 2024 10:51


          removing bascule/acquire


          Merge branch 'feat/webhook-update' into acquire

e1bb210


          draft: removing bascule/acquire

037c5bb

maurafortino added the needs discussion label

maurafortino requested a review from denopink

November 18, 2024 16:54

maurafortino self-assigned this

maurafortino marked this pull request as draft

November 18, 2024 16:54

denopink reviewed

View reviewed changes

chrysom/basicClient.go Outdated Show resolved Hide resolved

denopink reviewed

View reviewed changes

service.go Outdated Show resolved Hide resolved

denopink reviewed

View reviewed changes

acquire/acquire.go Outdated Show resolved Hide resolved

denopink reviewed

View reviewed changes

acquire/acquire.go Outdated Show resolved Hide resolved

denopink requested changes

View reviewed changes

Contributor

denopink left a comment

Great job! I left a couple of requests 🙂

denopink reviewed

View reviewed changes

chrysom/basicClient_test.go Show resolved Hide resolved

maurafortino commented

View reviewed changes

chrysom/basicClient.go Show resolved Hide resolved

denopink reviewed

View reviewed changes

acquire/acquire.go Outdated Show resolved Hide resolved

denopink reviewed

View reviewed changes

Contributor

denopink left a comment

Left some followup notes

maurafortino added 2 commits

November 19, 2024 17:01


          removed acquire package; moved aquire to chrysom; added back in auth …

d70a27f

…to basicclientconfig


          fixed the failing tests for acquire

Contributor Author

maurafortino commented Nov 20, 2024

@denopink there is one test failing in listenerClient_test.go.
do you want to merge this PR and create a new PR for fixing the test in that file? Or good to fix in same PR?

Contributor

denopink commented Nov 20, 2024

@maurafortino let's fix all failing tests that are related to this code change 🙂

maurafortino added 4 commits

November 21, 2024 13:25


          fixing test in listenerClient_test

ec340ab


          add if check for nil auth

a1d66ef


          fixed formatting

edca28c


          fixed formatting

44b0ca9

maurafortino commented

View reviewed changes

chrysom/basicClient.go

               	if err != nil {
               		return response{}, fmt.Errorf(errWrappedFmt, errNewRequestFailure, err.Error())
               	}
-              	err = acquire.AddAuth(r, c.auth)
+              	if c.auth == nil {

Contributor Author

maurafortino Nov 21, 2024

this fixes the failing test in listenerClient_test

maurafortino marked this pull request as ready for review

November 21, 2024 18:37

denopink approved these changes

View reviewed changes

Contributor

denopink left a comment

lgtm! 🍻

denopink merged commit 3980858 into feat/webhook-update

14 checks passed

denopink deleted the acquire branch

November 21, 2024 18:49

maurafortino mentioned this pull request

Ancla Updates xmidt-org/tr1d1um#589

Draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs discussion