Skip to content

Conversation

@rhatdan
Copy link
Member

@rhatdan rhatdan commented Jan 12, 2021

Add a .generate-bindings make target that only runs in the absence of
the .generate-bindings file or when a types.go file below
pkg/bindings has changed.

It does not execute anything yet is just a template to wire in the
generation in a future change.

Signed-off-by: Valentin Rothberg [email protected]
Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Jan 12, 2021

@baude @vrothberg @nalind PTAL

Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

Looks like the build tests are failing because generate now has a dependency on goimports being in $PATH now? If a user doesn't have goimports in their path, the imports will not be correctly done.

I also would prefer it if we kept the generated time in the generated files.

Also, generating the bindings touches go.mod and go.sum and throws a bunch of inconsistent vendoring errors at me. Not entirely sure what's going on, but that might be my fault.

The files are correctly generated though, so that's good

@rhatdan rhatdan force-pushed the Makefile branch 15 times, most recently from 2ab2fcb to c1e1faa Compare January 13, 2021 19:54
@rhatdan rhatdan force-pushed the Makefile branch 2 times, most recently from 886cfb1 to 925b0ac Compare January 14, 2021 09:03
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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

The pull request process is described here

Details 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

@rhatdan rhatdan force-pushed the Makefile branch 2 times, most recently from 5e9827b to 0277ddb Compare January 14, 2021 09:14
Copy link
Member

Choose a reason for hiding this comment

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

why did you remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

gofmt is not available in path on a lot of ci/cd images, go fmt seems to work, so I switched to it.

 go fmt [-n] [-x] [packages]

Fmt runs the command 'gofmt -l -w' on the packages named
by the import paths. It prints the names of the files that are modified.

For more about gofmt, see 'go doc cmd/gofmt'.
For more about specifying packages, see 'go help packages'.

The -n flag prints commands that would be executed.
The -x flag prints commands as they are executed.

The -mod flag's value sets which module download mode
to use: readonly or vendor. See 'go help modules' for more.

To run gofmt with specific options, run gofmt itself.

See also: go fix, go vet.

Copy link
Member

Choose a reason for hiding this comment

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

why did you remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this will cause us to have to update the bindings every time you run create bindings even if the bindings did not change.

Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

go generate does this not, me.

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 likely your problem. looking into why.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, this gets removed when you go generate, I tried to hard code this back in, but it caused other tools to break.
Strange that I can not get this same error to happen with podman-remote ps --last 1.

Add a `.generate-bindings` make target that only runs in the absence of
the `.generate-bindings` file or when a `types.go` file below
`pkg/bindings` has changed.

This will regenerate the go bindings and make sure the code is up2date.

Signed-off-by: Valentin Rothberg <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

LGTM assuming tests pass!

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.

LGTM, thanks a lot @rhatdan !

@vrothberg
Copy link
Member

/lgtm hold

@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2021
@umohnani8
Copy link
Member

LGTM

@openshift-merge-robot openshift-merge-robot merged commit 2b7793b into containers:master Jan 14, 2021
@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.

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

Development

Successfully merging this pull request may close these issues.

8 participants