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

Fix addMemberAsLearnerAndPromote to avoid error 'etcdserver: can only… #19279

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Jan 26, 2025

… promote a learner member'

Fix a test regression caused by #19272

This is another example of regression caused by "ok-to-test" label wasn't added.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

… promote a learner member'

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr
Copy link
Member Author

ahrtr commented Jan 26, 2025

This is another example of regression caused by "ok-to-test" label wasn't added.

@ivanvc @jmhbnz Is it possible to prevent this next time? For example, add a workflow to check the "ok-to-test" label, if not set, then fails the workflow?

@ahrtr
Copy link
Member Author

ahrtr commented Jan 26, 2025

See #19272 (comment)

Copy link

codecov bot commented Jan 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.92%. Comparing base (532c601) to head (c578649).
Report is 2 commits behind head on main.

Additional details and impacted files

see 19 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19279      +/-   ##
==========================================
+ Coverage   68.87%   68.92%   +0.05%     
==========================================
  Files         420      420              
  Lines       35680    35680              
==========================================
+ Hits        24573    24591      +18     
+ Misses       9685     9668      -17     
+ Partials     1422     1421       -1     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 532c601...c578649. Read the comment docs.

@ahrtr
Copy link
Member Author

ahrtr commented Jan 26, 2025

/test pull-etcd-integration-1-cpu-arm64

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, serathius

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

@serathius serathius merged commit f7a36a5 into etcd-io:main Jan 27, 2025
36 checks passed
@ahrtr ahrtr deleted the test_regression_20250126 branch January 27, 2025 09:06
@ivanvc
Copy link
Member

ivanvc commented Jan 27, 2025

This is another example of regression caused by "ok-to-test" label wasn't added.

@ivanvc @jmhbnz Is it possible to prevent this next time? For example, add a workflow to check the "ok-to-test" label, if not set, then fails the workflow?

I'm unsure if it will be possible to add a workflow, as workflows still need approval to run. I think a better approach would be prioritizing migrating all remaining workflows to prow jobs and eventually enabling tide.

@ahrtr
Copy link
Member Author

ahrtr commented Jan 27, 2025

I'm unsure if it will be possible to add a workflow, as workflows still need approval to run.

Something like below?

name: Check ok-to-test Label

on:
  pull_request:
    types:
      - opened
      - edited
      - synchronize
      - labeled
      - unlabeled

jobs:
  check-label:
    runs-on: ubuntu-latest
    steps:
      # Step 1: Checkout the code (required for most workflows)
      - name: Checkout code
        uses: actions/checkout@v3

      # Step 2: Fetch PR information
      - name: Get PR labels
        id: get-labels
        uses: actions/github-script@v6
        with:
          script: |
            const labels = context.payload.pull_request.labels.map(label => label.name);
            core.setOutput("labels", labels);

      # Step 3: Verify if the label 'ok-to-test' exists
      - name: Check for 'ok-to-test' label
        run: |
          echo "Labels found: ${{ steps.get-labels.outputs.labels }}"
          if [[ "${{ steps.get-labels.outputs.labels }}" != *"ok-to-test"* ]]; then
            echo "Error: 'ok-to-test' label is missing."
            exit 1
          fi

@ivanvc
Copy link
Member

ivanvc commented Jan 27, 2025

@ahrtr, yes, but the issue is that we'll need someone to approve the workflows manually (which is what gh-workflow-approve.yaml//ok-to-test is doing):

for (var run of result.data.workflow_runs) {
await github.rest.actions.approveWorkflowRun({
owner: context.repo.owner,
repo: context.repo.repo,
run_id: run.id
});

For Prow, having the ok-to-test label is enough to kick off the jobs, but we need that workflow for GitHub.

So, if we add a workflow as you suggest, we will still need to approve the workflow somehow. Therefore, having a workflow like that sounds redundant if it can't run unless someone approves it.

@ahrtr
Copy link
Member Author

ahrtr commented Jan 28, 2025

So, if we add a workflow as you suggest, we will still need to approve the workflow somehow.

Probably I missed something. If we just run the workflow (Check ok-to-test Label) in github instead of prow, then it should work?

@ivanvc
Copy link
Member

ivanvc commented Jan 28, 2025

Please look at this pull request from a first-time contributor: #19123. It doesn't run any GitHub workflows unless someone approves them. Therefore, a workflow to check if the ok-to-test label is present won't work.

@ivanvc
Copy link
Member

ivanvc commented Jan 28, 2025

However, for regular contributors (still not team members), it will still trigger the workflows, i.e., #18711. I guess it would still be an improvement. Would you like to pursue this workflow?

@ahrtr
Copy link
Member Author

ahrtr commented Jan 28, 2025

i.e., #18711. I guess it would still be an improvement. Would you like to pursue this workflow?

YES, it's exactly the motivation of the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants