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 test e2e by ensuring that make manifests and generate are executed as part of the tests #4122

Merged

Conversation

camilamacedo86
Copy link
Member

We are running make manifests and make generate in the github action when those should be called by the e2e tests since we can trigger those locally, for example, from IDE to troubleshooting

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 31, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title fix: test e2e should run make commands 🐛 (go/v4): fix test e2e by ensuring that run make commands are executed prior the tests Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 (go/v4): fix test e2e by ensuring that run make commands are executed prior the tests 🐛 (go/v4): fix test e2e by ensuring that make manifests and generate are executed as part of the tests Aug 31, 2024
@camilamacedo86
Copy link
Member Author

c/c @Adembc

We are running make manifests and make generate in the github action
when those should be called by the e2e tests since we can trigger those
locally, for example, from IDE to troubleshooting
@camilamacedo86 camilamacedo86 force-pushed the add-make-commands-to-e2e branch from 3b860ce to 107b809 Compare August 31, 2024 11:05
@camilamacedo86
Copy link
Member Author

I am moving forward because it is a nit/fix
We have big fishes to fry and we need help with a lot of PRs reviews.
So, I will not make nobody waste time on this one.

Also, anyone can propose further changes in follow ups
All help is welcome !
Please, feel free.

@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3570efc into kubernetes-sigs:master Aug 31, 2024
17 of 18 checks passed
@camilamacedo86 camilamacedo86 deleted the add-make-commands-to-e2e branch August 31, 2024 16:18
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Sep 10, 2024
Apply the fix introduced in PR kubernetes-sigs#4122. The default markers in the code
have been replaced with the values defined in the hack/docs, but this
change was missed in the tutorial samples.
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Sep 11, 2024
Apply the fix introduced in PR kubernetes-sigs#4122. The default markers in the code
have been replaced with the values defined in the hack/docs, but this
change was missed in the tutorial samples.
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Sep 11, 2024
…ithout overwriting webhook markers

Ensure we are more precise with  operations to avoid overwriting the default scaffolding. Previously, the markers in the scaffold were unintentionally overwritten when injecting code and documentation, which could override crucial changes.

This commit ensures the fix applied in PR kubernetes-sigs#4122 is preserved by refining the tutorial instructions to avoid overwriting webhook markers while still injecting the necessary information.

Either we should not export the consts used in the hack/docs since those are internal implementations, therefore the consts used in this area affected have their name changed
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Sep 11, 2024
…ithout overwriting webhook markers

Ensure we are more precise with  operations to avoid overwriting the default scaffolding. Previously, the markers in the scaffold were unintentionally overwritten when injecting code and documentation, which could override crucial changes.

This commit ensures the fix applied in PR kubernetes-sigs#4122 is preserved by refining the tutorial instructions to avoid overwriting webhook markers while still injecting the necessary information.

Either we should not export the consts used in the hack/docs since those are internal implementations, therefore the consts used in this area affected have their name changed
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Sep 11, 2024
…ithout overwriting webhook markers

Ensure we are more precise with  operations to avoid overwriting the default scaffolding. Previously, the markers in the scaffold were unintentionally overwritten when injecting code and documentation, which could override crucial changes.

This commit ensures the fix applied in PR kubernetes-sigs#4122 is preserved by refining the tutorial instructions to avoid overwriting webhook markers while still injecting the necessary information.

Either we should not export the consts used in the hack/docs since those are internal implementations, therefore the consts used in this area affected have their name changed
camilamacedo86 added a commit that referenced this pull request Sep 11, 2024
…of explanations (#4155)

fix: improve CronJob and multiversion tutorial by refining replaces without overwriting webhook markers

Ensure we are more precise with  operations to avoid overwriting the default scaffolding. Previously, the markers in the scaffold were unintentionally overwritten when injecting code and documentation, which could override crucial changes.

This commit ensures the fix applied in PR #4122 is preserved by refining the tutorial instructions to avoid overwriting webhook markers while still injecting the necessary information.

Either we should not export the consts used in the hack/docs since those are internal implementations, therefore the consts used in this area affected have their name changed
@sebastian-philipp
Copy link

sebastian-philipp commented Feb 4, 2025

I'm actually not super happy with this.

  1. It make it harder to run the e2e suite locally. (I.e. you now need to do some intricate changes in Go to run it without generate and manifests and docker-build)
  2. People wondering, as we have this makefile target:
.PHONY: test-e2e
test-e2e: manifests generate fmt vet

why not just add docker-build as an additional dependency to test-e2e

  1. It makes the scaffolding harder to reason. (make calls Go. Go calls make again etc.)

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

Successfully merging this pull request may close these issues.

3 participants