-
Notifications
You must be signed in to change notification settings - Fork 253
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
🐛 fixup Makefile, should fix image build on master #516
🐛 fixup Makefile, should fix image build on master #516
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
6b08506
to
659a09c
Compare
/assign @hidekazuna |
/assign @prankul88 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, thanks.
@hidekazuna all done. Thx for the review |
@sbueringer Thanks, confirmed. |
@sbueringer Will review this tomorrow asap. |
looks good, just need other folks to take a quick look |
@sbueringer Can you update the version of kubebuilder Line 7 in b406c51
|
Otherwise LGTM |
@@ -295,20 +298,7 @@ release: clean-release ## Builds and push container images using the latest git | |||
|
|||
.PHONY: release-manifests | |||
release-manifests: $(RELEASE_DIR) ## Builds the manifests to publish with a release | |||
kustomize build config > $(RELEASE_DIR)/infrastructure-components.yaml | |||
|
|||
.PHONY: release-binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbueringer Was curious to know why do we not need release-binary
in release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why it was added. release-binary is used in CAPA to build clusterawsadm. But we don't have a binary like this for now, so we don't need the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. Thanks
I would like to keep it as is. The acutal version is defined in hack/tools/go.mod and is sync with CAPA (controller-tools). Not sure what consequences it have to use something else so I would just like to keep it sync for now EDIT: Sorry you're right. had an old version of CAPA locally. They changed it like you said. I'll push a fix commit |
@hidekazuna @jichenjc @prankul88 |
LGTM. Others can confirm. :) |
looks good, let's try use it then submit further PRs :) |
Perfect :). Good news is I have time today to finalize the template PR. After that one we should have an almost final v1alpha3 version |
Cluster was created successfully in my env. /lgtm |
* fixup Makefile, should fix image build on master * remove unused v1alpha2 package, update doc, remove other references to v1alpha2 * Fixup cloudbuild.yaml * review fixes * update deps * re-gen
What this PR does / why we need it:
Syncs the Makefile with CAPA and should fix our build on master
Also:
As we're not really (imho) supporting migrations from v1alpha2 to v1alpha3 and the v1alpha2 package was unused in our implementation I would suggest removing it. I'm happy when we have v1alpha3 finalized I don't think we have the bandwidth to support migrations explicitly right now
See also: #499