Skip to content

Conversation

chansuke
Copy link
Member

Removed outdated check and simplify pull_optional() function.
The PR has been merged and the specific check for "alpha" tests and deployment suffix is no longer necessary.

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

Welcome @chansuke!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added 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 Aug 27, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @chansuke. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot requested review from msau42 and pohly August 27, 2023 09:59
@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config area/jobs sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 27, 2023
@rjsadow
Copy link
Contributor

rjsadow commented Aug 27, 2023

/ok-to-test

@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 Aug 27, 2023
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Running the modified script leads to changes in the jobs:

diff --git a/config/jobs/kubernetes-csi/csi-driver-host-path/csi-driver-host-path-config.yaml b/config/jobs/kubernetes-csi/csi-driver-host-path/csi-driver-host-path-config.yaml
index 1ce6641b69..53b5522725 100644
--- a/config/jobs/kubernetes-csi/csi-driver-host-path/csi-driver-host-path-config.yaml
+++ b/config/jobs/kubernetes-csi/csi-driver-host-path/csi-driver-host-path-config.yaml
@@ -296,7 +296,7 @@ presubmits:
   - name: pull-kubernetes-csi-csi-driver-host-path-alpha-1-25-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: []
@@ -347,7 +347,7 @@ presubmits:
   - name: pull-kubernetes-csi-csi-driver-host-path-1-24-test-on-kubernetes-1-24
     cluster: eks-prow-build-cluster
     always_run: true
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: []
@@ -444,7 +444,7 @@ presubmits:
   - name: pull-kubernetes-csi-csi-driver-host-path-1-25-test-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: true
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: []
@@ -638,7 +638,7 @@ presubmits:
   - name: pull-kubernetes-csi-csi-driver-host-path-alpha-1-25-test-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: []
diff --git a/config/jobs/kubernetes-csi/external-attacher/external-attacher-config.yaml b/config/jobs/kubernetes-csi/external-attacher/external-attacher-config.yaml
index 2aa58d51fd..874405cbd4 100644
--- a/config/jobs/kubernetes-csi/external-attacher/external-attacher-config.yaml
+++ b/config/jobs/kubernetes-csi/external-attacher/external-attacher-config.yaml
@@ -296,7 +296,7 @@ presubmits:
   - name: pull-kubernetes-csi-external-attacher-alpha-1-25-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: ["^(release-0.2.0|release-0.3.0|release-0.4|release-1.0|v0.1.0)$"]
diff --git a/config/jobs/kubernetes-csi/external-provisioner/external-provisioner-config.yaml b/config/jobs/kubernetes-csi/external-provisioner/external-provisioner-config.yaml
index c08bac9128..ee9a83f9b7 100644
--- a/config/jobs/kubernetes-csi/external-provisioner/external-provisioner-config.yaml
+++ b/config/jobs/kubernetes-csi/external-provisioner/external-provisioner-config.yaml
@@ -296,7 +296,7 @@ presubmits:
   - name: pull-kubernetes-csi-external-provisioner-alpha-1-25-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: ["^(release-0.2.0|release-0.3.0|release-0.4|release-1.0|v0.1.0)$"]
diff --git a/config/jobs/kubernetes-csi/external-resizer/external-resizer-config.yaml b/config/jobs/kubernetes-csi/external-resizer/external-resizer-config.yaml
index 30533365aa..eddabee8aa 100644
--- a/config/jobs/kubernetes-csi/external-resizer/external-resizer-config.yaml
+++ b/config/jobs/kubernetes-csi/external-resizer/external-resizer-config.yaml
@@ -296,7 +296,7 @@ presubmits:
   - name: pull-kubernetes-csi-external-resizer-alpha-1-25-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: []
diff --git a/config/jobs/kubernetes-csi/external-snapshotter/external-snapshotter-config.yaml b/config/jobs/kubernetes-csi/external-snapshotter/external-snapshotter-config.yaml
index 4b02474b9e..cda7dd60bd 100644
--- a/config/jobs/kubernetes-csi/external-snapshotter/external-snapshotter-config.yaml
+++ b/config/jobs/kubernetes-csi/external-snapshotter/external-snapshotter-config.yaml
@@ -296,7 +296,7 @@ presubmits:
   - name: pull-kubernetes-csi-external-snapshotter-alpha-1-25-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: ["^(k8s_1.12.0-beta.1|release-0.4|release-1.0)$"]
diff --git a/config/jobs/kubernetes-csi/livenessprobe/livenessprobe-config.yaml b/config/jobs/kubernetes-csi/livenessprobe/livenessprobe-config.yaml
index bce1948fd4..5d277464b0 100644
--- a/config/jobs/kubernetes-csi/livenessprobe/livenessprobe-config.yaml
+++ b/config/jobs/kubernetes-csi/livenessprobe/livenessprobe-config.yaml
@@ -296,7 +296,7 @@ presubmits:
   - name: pull-kubernetes-csi-livenessprobe-alpha-1-25-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: ["^(release-0.4|release-1.0)$"]
diff --git a/config/jobs/kubernetes-csi/node-driver-registrar/node-driver-registrar-config.yaml b/config/jobs/kubernetes-csi/node-driver-registrar/node-driver-registrar-config.yaml
index 4ece605cc4..47269f5013 100644
--- a/config/jobs/kubernetes-csi/node-driver-registrar/node-driver-registrar-config.yaml
+++ b/config/jobs/kubernetes-csi/node-driver-registrar/node-driver-registrar-config.yaml
@@ -296,7 +296,7 @@ presubmits:
   - name: pull-kubernetes-csi-node-driver-registrar-alpha-1-25-on-kubernetes-1-25
     cluster: eks-prow-build-cluster
     always_run: false
-    optional: true
+    optional: false
     decorate: true
     skip_report: false
     skip_branches: ["^(release-1.0)$"]

Is that intentional?

If yes, include them here. If not, the if checks in the script aren't obsolete yet.

@chansuke
Copy link
Member Author

@pohly
Yes, the changes to set the optional attribute to false are intentional. The referenced pull request kubernetes-csi/csi-driver-host-path/pull/282 has been merged, fulfilling the conditions outlined in the TODO comment.
Therefore, the jobs that depend on the new deployment flavors no longer need to be optional.

@chansuke chansuke requested a review from pohly August 28, 2023 09:53
@pohly
Copy link
Contributor

pohly commented Aug 28, 2023

Yes, the changes to set the optional attribute to false are intentional.

Then include them in this PR please.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 29, 2023
cluster: eks-prow-build-cluster
always_run: false
optional: true
optional: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I look at this again, I wonder whether this really makes sense. This is a job which runs alpha features. Should the result of those jobs really be merge-blocking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the feedback. I agree that this job, which runs alpha features, shouldn't necessarily be merge-blocking. I'll revert the script to its original state to reflect this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pohly I've reverted the chnges.
Do you think it's reasonable to keep the TODO comment? If not, I can remove it.

# https://github.com/kubernetes-csi/csi-driver-host-path/pull/282 has not been merged yet,
# therefore pull jobs which depend on the new deployment flavors have to be optional.
# TODO: remove this check once merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is out-dated, so it would make sense to update the script so that has the right checks and produces the desired output.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 29, 2023
@chansuke chansuke requested a review from pohly August 31, 2023 17:19
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Please include the resulting changes in the jobs (there are some!) and squash into a single commit.

@chansuke chansuke force-pushed the hotfix/remove-obsolete-alpha-check branch from eca854f to 8ef8930 Compare October 3, 2023 15:03
@k8s-ci-robot k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 3, 2023
@chansuke chansuke requested a review from pohly October 3, 2023 15:03
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 3, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2023
@chansuke chansuke force-pushed the hotfix/remove-obsolete-alpha-check branch from 8ef8930 to 6298d32 Compare October 15, 2023 04:12
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 15, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chansuke
Once this PR has been reviewed and has the lgtm label, please ask for approval from pohly and additionally assign petr-muller for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 area/prow Issues or PRs related to prow label Oct 15, 2023
@k8s-ci-robot
Copy link
Contributor

@chansuke: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-test-infra-unit-test 6298d32 link true /test pull-test-infra-unit-test
pull-test-infra-integration 6298d32 link true /test pull-test-infra-integration
pull-test-infra-unit-test-race-detector-nonblocking 6298d32 link false /test pull-test-infra-unit-test-race-detector-nonblocking
pull-test-infra-verify-lint 6298d32 link true /test pull-test-infra-verify-lint
pull-test-infra-prow-image-build-test 6298d32 link true /test pull-test-infra-prow-image-build-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 7, 2024
@chansuke chansuke closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Issues or PRs related to code in /config area/jobs area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants