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

Migrate to Go Modules and simplify Makefile #277

Merged
merged 1 commit into from Mar 27, 2020
Merged

Migrate to Go Modules and simplify Makefile #277

merged 1 commit into from Mar 27, 2020

Conversation

periklis
Copy link
Contributor

@periklis periklis commented Mar 23, 2020 •
edited

Motivation

This PR migrates dependency management from dep to go modules to enable forward versioned support for k8s >1.18 api, client-go, library-go. In addition the Makefile includes following enhancements:

  • Pin GOFLAGS to -mod=vendor for vendoring all go module dependencies.
  • Pin tool versions for operator-sdk, golangci-lint, gosec and imagebuilder
  • Consistent usage of tools from repository local GOBIN folder.
  • Generate k8s api, k8s crds and move to manifests folder
  • Adds a lint target for golangci-lint
  • Use golang-1.13 as builder base image.

jcantrill
jcantrill approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

One nit

.gitignore Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2020
@periklis periklis mentioned this pull request Mar 23, 2020
Merged
alanconway
alanconway reviewed Mar 23, 2020
View reviewed changes
Copy link
Contributor

@alanconway alanconway left a comment

Choose a reason for hiding this comment

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

Check the comments, I improved my approach to download/build commands.
Hurray for go mod! Lets do that to CLO as well once we can merge again...

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@periklis periklis force-pushed the go-modules branch 3 times, most recently from 70a57a8 to c125948 Compare March 23, 2020 17:26
ewolinetz
ewolinetz reviewed Mar 23, 2020
View reviewed changes
pkg/apis/logging/v1/kibana_types.go
@@ -32,8 +32,6 @@ type KibanaStatus struct {
Conditions map[string]ClusterConditions `json:"clusterCondition,omitempty"`
}

type ClusterConditions []ClusterCondition
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing this here but adding it in the elasticsearch_types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type aliases should live close to their referenced/aliased types. IMHO it enhances readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

so this wasn't being used here nor in elasticsearch_types before? why not just drop it here and leave it as that?

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 used in both types but ClusterCondition lived in elasticsearch_types.go while this alias in kibana_types.go

@periklis periklis force-pushed the go-modules branch 3 times, most recently from 0d7e2ed to 921f6f7 Compare March 24, 2020 13:13
ewolinetz
ewolinetz reviewed Mar 24, 2020
View reviewed changes
pkg/apis/logging/v1/elasticsearch_types.go
@@ -150,6 +150,8 @@ type ClusterCondition struct {
Message string `json:"message,omitempty" protobuf:"bytes,6,opt,name=message"`
}

type ClusterConditions []ClusterCondition
Copy link
Contributor

Choose a reason for hiding this comment

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

revert? re: discussion on kibana_types.go comment

@periklis periklis force-pushed the go-modules branch 2 times, most recently from 61b46a2 to cf4e603 Compare March 24, 2020 17:00
@periklis
Copy link
Contributor Author

/retest

@periklis periklis force-pushed the go-modules branch 4 times, most recently from c85a8fc to 2779ff4 Compare March 25, 2020 12:11
@periklis periklis mentioned this pull request Mar 25, 2020
Closed
jcantrill
jcantrill reviewed Mar 25, 2020
View reviewed changes
Makefile Outdated Show resolved Hide resolved
golangci.yaml Show resolved Hide resolved
@jcantrill
Copy link
Contributor

/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 Mar 25, 2020
@jcantrill
Copy link
Contributor

/lgtm

@jcantrill
Copy link
Contributor

/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 Mar 27, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, periklis

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 cf3ed38 into openshift:master Mar 27, 2020
@periklis periklis deleted the go-modules branch March 27, 2020 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewolinetz ewolinetz ewolinetz left review comments

@jcantrill jcantrill jcantrill approved these changes

@alanconway alanconway alanconway left review comments

Assignees

@jcantrill jcantrill

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@periklis @jcantrill @openshift-ci-robot @ewolinetz @alanconway @openshift-merge-robot