Skip to content

Clean TypeMeta's and ListMeta's description and force pruning #478

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
Merged
openshift-merge-robot merged 6 commits into from
Oct 18, 2019
Merged

Clean TypeMeta's and ListMeta's description and force pruning #478

openshift-merge-robot merged 6 commits into from
Oct 18, 2019

Conversation

@soltysh
Copy link
Contributor

@soltysh soltysh commented Oct 16, 2019

/assign @sttts

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 16, 2019
@soltysh soltysh force-pushed the object-meta branch 2 times, most recently from f5f6a9c to 97eb6ff Compare October 17, 2019 11:01
sttts
sttts reviewed Oct 17, 2019
View reviewed changes
config/v1/types.go
// configuration. If present, it will be used instead of the path to the configuration file.
// +nullable
// +kubebuilder:validation:PreserveUnknownFields
Configuration runtime.RawExtension `json:"configuration"`
Copy link
Contributor

@sttts sttts Oct 17, 2019

Choose a reason for hiding this comment

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

this is an embedded resource I believe.

@sttts
Copy link
Contributor

sttts commented Oct 17, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2019
@soltysh
Copy link
Contributor Author

soltysh commented Oct 17, 2019

/retest

@soltysh
Copy link
Contributor Author

soltysh commented Oct 17, 2019

/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 Oct 17, 2019
@soltysh
Copy link
Contributor Author

soltysh commented Oct 17, 2019

Now that openshift/release#5484 merged
/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2019
@soltysh
Copy link
Contributor Author

soltysh commented Oct 18, 2019

/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 Oct 18, 2019
sttts
sttts reviewed Oct 18, 2019
View reviewed changes
hack/alpha-build-machinery/make/targets/openshift/controller-gen.mk
CONTROLLER_GEN_TEMP ?=$(PERMANENT_TMP_GOPATH)/src/sigs.k8s.io/controller-tools
controller_gen_gopath =$(shell realpath -m $(CONTROLLER_GEN_TEMP)/../..)
CONTROLLER_GEN ?=$(CONTROLLER_GEN_TEMP)/controller-gen
CONTROLLER_GEN ?=$(PERMANENT_TMP_GOPATH)/bin/controller-gen
Copy link
Contributor

@sttts sttts Oct 18, 2019

Choose a reason for hiding this comment

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

why not give it a unique name? This way, when switching branches, you easily end up with the wrong generator.

Copy link
Contributor

@sttts sttts Oct 18, 2019

Choose a reason for hiding this comment

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

controller-gen-$(CONTROLLER_GEN_VERSION)

Copy link
Contributor Author

@soltysh soltysh Oct 18, 2019

Choose a reason for hiding this comment

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

Honestly, when switching branches make clean is needed, and make update in this repo is not that lengthy so I don't think that makes it worth the effort.

Copy link
Contributor

@sttts sttts Oct 18, 2019

Choose a reason for hiding this comment

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

Remember Tim Hockin's twitter comment about make semantics? Having to run make clean is such a violation.

sttts
sttts reviewed Oct 18, 2019
View reviewed changes
hack/alpha-build-machinery/make/targets/openshift/controller-gen.mk Outdated
cd '$(CONTROLLER_GEN_TEMP)' && export GO111MODULE=on GOPATH='$(controller_gen_gopath)' && $(GO) mod vendor 2>/dev/null && $(GO) build -mod=vendor ./cmd/controller-gen
$(info Installing controller-gen into '$(CONTROLLER_GEN)')
mkdir -p '$(controller_gen_dir)'
curl -s -f -L https://github.com/openshift/kubernetes-sigs-controller-tools/releases/download/v0.2.1-37-ga3cca5d/controller-gen-$(GOHOSTOS)-$(GOHOSTARCH) -o '$(CONTROLLER_GEN)'
Copy link
Contributor

@sttts sttts Oct 18, 2019

Choose a reason for hiding this comment

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

s/v0.2.1-37-ga3cca5d/$(CONTROLLER_GEN_VERSION)/

sttts
sttts reviewed Oct 18, 2019
View reviewed changes
hack/alpha-build-machinery/make/targets/openshift/controller-gen.mk
@@ -1,29 +1,22 @@
self_dir :=$(dir $(lastword $(MAKEFILE_LIST)))

CONTROLLER_GEN_VERSION ?=v0.2.1
Copy link
Contributor

@sttts sttts Oct 18, 2019

Choose a reason for hiding this comment

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

CONTROLLER_GEN_VERSION ?=v0.2.1-37-ga3cca5d

@sttts
Copy link
Contributor

sttts commented Oct 18, 2019

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 18, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, sttts

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

@openshift-merge-robot openshift-merge-robot merged commit d0b31d7 into openshift:master Oct 18, 2019
@soltysh soltysh deleted the object-meta branch October 18, 2019 14:03
@damemi damemi mentioned this pull request Oct 28, 2019
Merged
@wking
Copy link
Member

wking commented Oct 28, 2019

Context around this change here and here (metadata schema/values are centrally enforced, although I'm not sure what that means for oc explain ..., etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@bparees bparees Awaiting requested review from bparees

1 more reviewer

@sttts sttts sttts left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sttts sttts

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@soltysh @sttts @openshift-ci-robot @wking @openshift-merge-robot