Skip to content
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

[PL-26239]: fix for list response #218

Merged
merged 3 commits into from
Aug 26, 2022
Merged

[PL-26239]: fix for list response #218

merged 3 commits into from
Aug 26, 2022

Conversation

bhavya181
Copy link
Contributor

response schema for listUserRepositries and listAppnstallations was different, and that's why it was throwing error, so changes the output.
Tested with writing integartion test with my credentials, now, its working fine.

@@ -14,7 +14,7 @@ import (
"github.com/drone/go-scm/scm"
)

type repository struct {
type Repository struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, it makes the struct public. Why do we need to make it public if all references are from same package/file?
Sorry, I may be naive here, just trying to understand its impact

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

json unmarshal and marshal only works when name of struct start with capital letter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting. Thanks for the info.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

https://github.com//pull/219 repositories should be lower case as @mohitg0795 said

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then, RepositoryListResponse will not be able to map to Repository struct as it does not unmarshal that struct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you suggest any other method, so that it will map correctly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you show me where it doesnt work, the unit tests passed and it unmarshalled successfully

if you look at the do method for all drivers, it marshalls / unmarshalls without all of the structs. The other structs all start with a lowercase letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, I have changed them now

@@ -14,7 +14,7 @@ import (
"github.com/drone/go-scm/scm"
)

type repository struct {
type Repository struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

https://github.com//pull/219 repositories should be lower case as @mohitg0795 said

@@ -53,6 +53,11 @@ type hook struct {
} `json:"config"`
}

type RepositoryListResponse struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should start with a lowercase letter

Copy link
Member

@bradrydzewski bradrydzewski Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree this should be lowercase. Also we do not suffix with Response so it should be repositoryList instead. If this package already has a repositoryList struct, you could alternative use the name repositoryListByInstallation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

@bradrydzewski
Copy link
Member

bradrydzewski commented Aug 26, 2022

The repository struct should never be exposed as public, it should be kept private and converted to scm.Repository. The purpose of this library is to convert vendor-specific structures to common abstractions. The scm.Repository is the common abstraction that must be used.

If the response structure is different, you should create a new repositoryList structure instead. Then you would create a convertRepositoryListByInstallation function that converts the repositories in the repositoryList to a []scm.Repository structure.

@tphoney tphoney merged commit f4a3110 into drone:master Aug 26, 2022
@tphoney tphoney added the bug label Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants