Bug 1871858: update make generate #159
Conversation
michaelgugino
force-pushed
the
update-make-gen
branch
from
4423123 to
e991eb6
Compare
|
@alexander-demichev @elmiko |
| ./hack/update-generated-deep-copies.sh | ||
| generate: | ||
| $(DOCKER_CMD) go generate ./pkg/... ./cmd/... | ||
| hack/goimports.sh . |
There was a problem hiding this comment.
when i run this target in no docker mode, make will fail because the git diff returns a non zero value. i think we need to adjust how this gets returned when not using containerized builds.
There was a problem hiding this comment.
@elmiko
Docker mode is source of truth, that's what gets run in CI. The output of go's tooling can change from version to version. It's a mess, so best practice is to use the docker containers for reproducible stuff.
There was a problem hiding this comment.
got it. i feel like we should just remove the non-container builds if we aren't going to maintain the "NO_DOCKER" option.
There was a problem hiding this comment.
We need the "NO_DOCKER" option because the CI uses it. The jobs are already running in the container, so we pass them NO_DOCKER, so we can't get rid of it. Having Docker in the Makefile is for us to run it locally in the same environment as the CI.
There was a problem hiding this comment.
yeah it makes sense, i totally get why we use docker builds for stuff. i'd just like to see these makefiles become more bulletproof for developers. i find the callout to docker specifically annoying because i don't have it, or an alias, on many machines since it's been removed from more recent fedora versions.
and this is not an issue for this PR, i'm just griping more in general now because it seems like some of these makefiles breakdown when you try to run them locally, and i find it to be a pain for devel stuff.
There was a problem hiding this comment.
Why do we we have to run goimports after doing the code generation? That could maybe use a comment as I don't think it's obvious
There was a problem hiding this comment.
@JoelSpeed I don't know. This is just copying what we're doing on AWS. openshift/cluster-api-provider-aws#258
There was a problem hiding this comment.
Ack, and there's no explanation of why that is on the AWS side either 😞
|
i'm fine to approve this, @michaelgugino would you mind adding a description to the commit message so we know why this change happened? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elmiko 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 |
openshift-ci-robot
added
the
approved
michaelgugino
force-pushed
the
update-make-gen
branch
from
e991eb6 to
2092978
Compare
This commit updates Make generate target to make it uniform with other providers. Current Make generate does not work correctly. This commit removes obselete hack script for generating.
michaelgugino
force-pushed
the
update-make-gen
branch
from
2092978 to
7cde6e2
Compare
|
i tested this out locally with a symlink from docker to podman, everything seemed to work as expected. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@alexander-demichev @elmiko |
it looks like we might need to create a bugzilla at this point given where we are in the release cycle, or if there is a bz already we just need to adjust the title slightly. |
openshift-ci-robot
added
bugzilla/severity-unspecified
|
@michaelgugino: This pull request references Bugzilla bug 1871858, which is invalid:
Comment In response to this:
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. |
| ./hack/update-generated-deep-copies.sh | ||
| generate: | ||
| $(DOCKER_CMD) go generate ./pkg/... ./cmd/... | ||
| hack/goimports.sh . |
There was a problem hiding this comment.
Ack, and there's no explanation of why that is on the AWS side either 😞
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/severity-low
|
@michaelgugino: This pull request references Bugzilla bug 1871858, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
23 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test e2e-azure-operator |
|
Seems as though this failure is unrelated to this PR. Looks like the scale down and cleanup timed out after 360s. |
|
@michaelgugino: All pull requests linked via external trackers have merged: Bugzilla bug 1871858 has been moved to the MODIFIED state. In response to this:
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. |
|
@michaelgugino: #159 failed to apply on top of branch "release-4.5": In response to this:
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. |
This commit updates Make generate target to make it
uniform with other providers. Current Make generate
does not work correctly.
This commit removes obselete hack script for generating.