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

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

Conversation

lukas016
Copy link
Contributor

Description:
current implementation of makefile target build-installer cannot work properly without crd. Root cause of problem is missing discard of content, if crd isn't used.
Result of problem is appending new content behind previous into dist/install.yaml and file content is growing and contains multiple versions.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 31, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @lukas016. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@lukas016 lukas016 marked this pull request as ready for review January 31, 2024 10:35
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2024
@lukas016
Copy link
Contributor Author

lukas016 commented Jan 31, 2024

@camilamacedo86 it is probably your code, could you pls do review too. Thx

fi
echo "---" >> dist/install.yaml # Add a document separator before appending
Copy link
Member

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

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

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 31, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 clean content of dist/install.yaml before update 🐛 (go/v4) Fix makefile target build-installer to discarding the previous content Jan 31, 2024
@lukas016 lukas016 force-pushed the bugfix-builder-installer-concatenation branch from f2e7c2c to 21554f2 Compare January 31, 2024 14:16
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 31, 2024
@lukas016 lukas016 force-pushed the bugfix-builder-installer-concatenation branch from 21554f2 to 3582cd3 Compare January 31, 2024 14:23
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2024
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approved

@lukas016

It is passing now, So all fine.
If you see any scenario that does not work please feel free to propose changes.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, lukas016

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2024
@k8s-ci-robot k8s-ci-robot merged commit d13a95f into kubernetes-sigs:master Jan 31, 2024
19 checks passed
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants