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

🐛 (go/v4) Fix makefile target build-installer to discarding the previous content #3766

Merged
merged 1 commit into from
Jan 31, 2024
Merged

🐛 (go/v4) Fix makefile target build-installer to discarding the previous content #3766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/book/src/component-config-tutorial/testdata/project/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
3 changes: 2 additions & 1 deletion docs/book/src/cronjob-tutorial/testdata/project/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
3 changes: 2 additions & 1 deletion pkg/plugins/golang/v4/scaffolds/internal/templates/makefile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
Copy link
Member

@camilamacedo86 camilamacedo86 Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@lukas016

Thank you for your contribution 🥇

/ok-to-test

Can you please run make generate?
The changes here will change what is outputted; therefore, we must run ' make generate`. (please also ensure that we have all in one commit) It is also tested via e2e tests, so passing on that is ALL fine.

Copy link
Member

Choose a reason for hiding this comment

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

It is either failing in teh ew tests

     mkdir -p dist
      echo "---" > dist/install.yaml # Clean previous content
      bash: -c: line 4: syntax error: unexpected end of file
      make: *** [Makefile:122: build-installer] Error 2
      
      {
          s: "make build-installer IMG=e2e-test/controller-manager:nfca failed with error: (exit status 2) /home/prow/go/src/sigs.k8s.io/kubebuilder/test/e2e/v4/e2e-nfca/bin/controller-gen-v0.14.0 rbac:roleName=manager-role crd webhook paths=\"./...\" output:crd:artifacts:config=config/crd/bases\n/home/prow/go/src/sigs.k8s.io/kubebuilder/test/e2e/v4/e2e-nfca/bin/controller-gen-v0.14.0 object:headerFile=\"hack/boilerplate.go.txt\" paths=\"./...\"\nDownloading sigs.k8s.io/kustomize/kustomize/v5@v5.3.0\nmkdir -p dist\necho \"---\" > dist/install.yaml # Clean previous content\nbash: -c: line 4: syntax error: unexpected end of file\nmake: *** [Makefile:122: build-installer] Error 2\n",

Can you please let me know the scenario that is not working for you?
Can you describe the steps to create a POC and reproduce the issue?
It would be great if we ensure that we also test it out in the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, i am working on fix.

Problematic scenario is:

  1. Remove config/crd
  2. Call make build-installer 2 or more times
  3. Check dist/install.yaml

Copy link
Member

Choose a reason for hiding this comment

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

Note that when you scaffold any API the tool in uncomment config/default/kustomization.yaml
You probably would solve it by commenting again on the

Resources:
- ../crd

In config/default/kustomization.yaml

So, that is not a really issue because
You break the scaffold if you manually remove the config/crd directory.
Are you able to face any issues without deleting things scaffold manually?

Nevertheless, if you want to change it to address it, it is OK, assuming it is still working as it should for the other scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we need if statement in makefile in this case? We can completely remove if statement and we won't need this fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this is better or why crd was generated independent?

build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
	mkdir -p dist
	cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
	$(KUSTOMIZE) build config/default > dist/install.yaml

Copy link
Member

@camilamacedo86 camilamacedo86 Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The above will not work:

  • a) Because you can create a project without CRD ( the problem is when you create the API, the scaffold change, so if you delete the config/crd manually, you also need to fix the scaffold manually accordingly )

Can you do the following to see if your problem is not fixed?

Remove config/crd
Comment `Resources:
- ../crd`
Call make build-installer 2 or more times
Check dist/install.yaml

OR

If you create a project and do not call kb create API, can you see any issue?

Furthermore, if you can face ANY issue using only the tool and without creating things within and then deleting staff manually or customizations, please let us know so that is an issue.

  • b) My 2 cents is that it will fail because it will be missing echo "---" >> dist/install.yaml # Add a document separator before appending

cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
3 changes: 2 additions & 1 deletion testdata/project-v4-multigroup-with-deploy-image/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
3 changes: 2 additions & 1 deletion testdata/project-v4-multigroup/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
3 changes: 2 additions & 1 deletion testdata/project-v4-with-deploy-image/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
3 changes: 2 additions & 1 deletion testdata/project-v4-with-grafana/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
3 changes: 2 additions & 1 deletion testdata/project-v4/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ docker-buildx: ## Build and push docker image for the manager for cross-platform
.PHONY: build-installer
build-installer: manifests generate kustomize ## Generate a consolidated YAML with CRDs and deployment.
mkdir -p dist
echo "---" > dist/install.yaml # Clean previous content
@if [ -d "config/crd" ]; then \
$(KUSTOMIZE) build config/crd > dist/install.yaml; \
echo "---" >> dist/install.yaml; \
fi
echo "---" >> dist/install.yaml # Add a document separator before appending
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/default >> dist/install.yaml

Expand Down
Loading