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.

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

Refactor generated code #9348

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Refactor generated code #9348

merged 1 commit into from
Feb 15, 2021

Conversation

matejvasek
Copy link
Contributor

@matejvasek matejvasek commented Feb 12, 2021 •
edited
Loading

Extracted common functionality to util function.

@matejvasek
Copy link
Contributor Author

Possible different approach is shown in: #9317

@matejvasek
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2021
@matejvasek matejvasek changed the title Refactor generated code [WIP] Refactor generated code Feb 12, 2021
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2021
@matejvasek
Copy link
Contributor Author

Ah again this nil != nil golang s#it.

@matejvasek
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2021
@matejvasek matejvasek changed the title [WIP] Refactor generated code Refactor generated code Feb 12, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2021
@TomSweeneyRedHat
Copy link
Member

@matejvasek if you can get this through tests, this will be the best PR of the year so far. Very nice work.

@matejvasek
Copy link
Contributor Author

@TomSweeneyRedHat I think that the only reason it was failing was that nil != nil thing. Also you might checkout completely different approach to this: #9317 however it's much more complex / less practical, getting rid of reflection is not really worth it probably.

@matejvasek
Copy link
Contributor Author

@TomSweeneyRedHat are those flakes?

vrothberg
vrothberg reviewed Feb 13, 2021
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

+281 −5,213

Nice work!

pkg/bindings/util/util.go Outdated
)

func IsSimpleType(f reflect.Value) bool {
func isSimpleType(f reflect.Value) bool {
Copy link
Member

Choose a reason for hiding this comment

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

This is technically an API break. While I doubt anybody is using it, I would sleep better keeping the two functions exported.

Retrospectively, it may should have been an pkg/bindings/*internal*/util where we can change whenever we want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced that just 10 ago so it's probably not used by anybody. I really should have added to some package with internal in its name.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I should have checked if it's in v3.0!

In this case, could you move this code into pkg/bindings/internal/util? This way, it's not part of the public API and we can change as much as we want without having to worry about potential external users.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the code can be moved in another PR. It's (almost) green, so we can merge this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was merged 10 days ago to master, and then backported to v3. So it was released 3 days ago with v3.

@vrothberg
Copy link
Member

@TomSweeneyRedHat are those flakes?

I restarted the failed jobs. It looked like flakes to me.

@matejvasek
Copy link
Contributor Author

Did just sys podman fedora-32 rootless host failed again.

@matejvasek
Copy link
Contributor Author

@vrothberg one of the two failing tests works not but the other one sys podman fedora-32 rootless host failed again 😞

@vrothberg
Copy link
Member

@vrothberg one of the two failing tests works not but the other one sys podman fedora-32 rootless host failed again disappointed

I'll restarted another time :) If it happens another time, I'll take a closer look.

@Luap99
Copy link
Member

Luap99 commented Feb 14, 2021

@matejvasek Can you please squash your commits and add a proper commit message.

@matejvasek
[NO TESTS NEEDED] Refactor generated code
6ffd05d 
Extracted common functionality to util function.

Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
Copy link
Contributor Author

Flake again.

@vrothberg
Copy link
Member

Restarted flakes

/hold
Holding the merge since I am still rather objecting to merge. If we want to, please wait a while until v3.0 further stabilized to avoid additional work for backports.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2021
@vrothberg
Copy link
Member

Restarted flakes

/hold
Holding the merge since I am still rather objecting to merge. If we want to, please wait a while until v3.0 further stabilized to avoid additional work for backports.

Apologies, I mixed tabs and meant to comment this in #9350

@vrothberg
Copy link
Member

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2021
@rhatdan
Copy link
Member

rhatdan commented Feb 15, 2021

/aproove
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2021
@rhatdan rhatdan added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: matejvasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 30607d7 into containers:master Feb 15, 2021
@edsantiago edsantiago mentioned this pull request Feb 15, 2021
Closed
@edsantiago edsantiago mentioned this pull request Feb 22, 2021
Closed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@matejvasek @TomSweeneyRedHat @vrothberg @Luap99 @rhatdan @openshift-ci-robot @openshift-merge-robot