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.

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

[OCPCLOUD-809] Add verify-diff check in generate task and enable in CI #744

Merged

Conversation

Danil-Grigorev
Copy link
Contributor

@Danil-Grigorev Danil-Grigorev commented Nov 4, 2020

This PR is dependent on openshift/release#13400 to enable the functionality in CI. Should go prior to the PR, allowing CI to get green.

Changes:

  • Check file diff between committed code and generated bits with hack/verify-diff.sh
  • go generate is moved into it's own script, which uses docker image with goimports tool in it - hack/go-gen.sh

@Danil-Grigorev
Copy link
Contributor Author

/retest

- Check file diff between committed code and generated bits with hack/verify-diff.sh
- go generate is moved into it's own script, which uses docker image with goimports tool in it - hack/go-gen.sh
@Danil-Grigorev Danil-Grigorev changed the title Add proper generate task for CI jobs [OCPCLOUD-809] Add verify-diff check in generate task and enable in CI Nov 5, 2020
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demichev

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020

REPO_NAME=$(basename "${PWD}")
if [ "$IS_CONTAINER" != "" ]; then
go generate ./pkg/apis/...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running this line forcefully in a container, as the goimports tool is a requirement for the environment, but a user/CI should not nessesary have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may be able to workaround this by running it from the vendor directory, do you know where the go generate command is in the code that is causing us to need goimports?

# creates. A simple `go generate` is enough for these.
# Also use this for generating deepcopy for all types (so that we use the same generator).
echo "Generating deepcopy funcs"
go generate ./pkg/apis/...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a part of codegen workflow, but was failing for me using the default docker comand coming from Makefile. Now it is a separate task managed by Makefile in https://github.com/openshift/machine-api-operator/pull/744/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R68

@Danil-Grigorev
Copy link
Contributor Author

/hold need to remove all docker references in CI run before merging

@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 Nov 5, 2020
@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Nov 5, 2020

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-azure-operator c2d0175 link /test e2e-azure-operator

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.

@Danil-Grigorev
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 Nov 5, 2020
@Danil-Grigorev
Copy link
Contributor Author

/retest
Removed all docker mentions in CI by adding IS_CONTAINER=TRUE NO_DOCKER=1 to the generate task. Otherwise the MAO job fails, see https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_release/13400/rehearse-13400-pull-ci-openshift-machine-api-operator-master-generate/1324352101890920448

Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

i tried this out locally a few times and everything seemed to work as i would expect.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020
@openshift-merge-robot openshift-merge-robot merged commit 1842a55 into openshift:master Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants