-
Notifications
You must be signed in to change notification settings - Fork 133
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
ROX-18155: pg generic store: GetAll #6769
ROX-18155: pg generic store: GetAll #6769
Conversation
Images are ready for the commit at ffe86f7. To use with deploy scripts, first |
6ec3701
to
c1557dc
Compare
16b5b7d
to
a6dd70b
Compare
c1557dc
to
29b04c2
Compare
a6dd70b
to
390bc82
Compare
29b04c2
to
5fb855e
Compare
390bc82
to
d6e5a0a
Compare
5fb855e
to
fe331f0
Compare
d6e5a0a
to
a315299
Compare
fe331f0
to
d78d2af
Compare
a315299
to
f501f43
Compare
@@ -729,21 +729,6 @@ func (s *storeImpl) GetIDs(ctx context.Context) ([]{{$singlePK.Type}}, error) { | |||
} | |||
{{- end }} | |||
|
|||
{{- if .GetAll }} |
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.
If not already done in the cleanup PR, make sure the get-all option is removed from the generator code as well as from the gen.go files that use it.
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.
The complete cleanup would impact:
- the gen.go files next to the stores generated by this pull request
- tools/generate-helpers/pg-table-bindings/migration.go.tpl (or the file could be removed completely, as well as the gen.go file in the migrator)
- tools/generate-helpers/pg-table-bindings/store.go.tpl (but I think the current pull request already removes all occurrences)
- tools/generate-helpers/pg-table-bindings/store_test.go.tpl (to let all stores test the GetAll function)
- tools/generate-helpers/pg-table-bindings/main.go
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.
So you think we should expose GetAll
method to all stores? I wander what was the reason to have it as optin
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.
d78d2af
to
ad19111
Compare
f501f43
to
c14880c
Compare
ad19111
to
53be42e
Compare
633cc54
to
96d90a5
Compare
@janisz: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Graphite rebased this pull request as part of a merge. |
96d90a5
to
ffe86f7
Compare
Description
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why
you did not do so. Valid reasons include, for example, "CI is sufficient",
"No testable changes". Feel free to attach JSON snippets, curl commands,
screenshots.
In addition to reviewing your code, reviewers must also review your testing
instructions and make sure they are sufficient.