-
Notifications
You must be signed in to change notification settings - Fork 237
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
Conversation
scm/driver/github/repo.go
Outdated
@@ -14,7 +14,7 @@ import ( | |||
"github.com/drone/go-scm/scm" | |||
) | |||
|
|||
type repository struct { | |||
type Repository struct { |
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.
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
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.
json unmarshal and marshal only works when name of struct start with capital letter
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.
Oh interesting. Thanks for the info.
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.
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.
but then, RepositoryListResponse will not be able to map to Repository struct as it does not unmarshal that struct
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.
Can you suggest any other method, so that it will map correctly
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.
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.
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.
ya, I have changed them now
scm/driver/github/repo.go
Outdated
@@ -14,7 +14,7 @@ import ( | |||
"github.com/drone/go-scm/scm" | |||
) | |||
|
|||
type repository struct { | |||
type Repository struct { |
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.
scm/driver/github/repo.go
Outdated
@@ -53,6 +53,11 @@ type hook struct { | |||
} `json:"config"` | |||
} | |||
|
|||
type RepositoryListResponse struct { |
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.
should start with a lowercase letter
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.
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
.
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.
changed
The If the response structure is different, you should create a new |
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.