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

[ci]: Fix auto-labeling #4276

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

s8sato
Copy link
Contributor

@s8sato s8sato commented Feb 13, 2024

Description

  • Fix: api-changes config-changes auto-labeling
  • Refactor: merge the above and iroha2 into one workflow
  • Feature: notify someone specific based on the above auto-labels
  • Feature: add another group of auto-labels to categorize PRs based on title

Linked issue

Benefits

  • Make PR labels easier to handle
  • Reduce workflows by one

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Feb 13, 2024
.github/workflows/iroha2-dev-pr-label.yml Outdated Show resolved Hide resolved
.github/workflows/iroha2-dev-pr-label.yml Outdated Show resolved Hide resolved
@s8sato s8sato marked this pull request as draft February 13, 2024 07:12
@0x009922
Copy link
Contributor

Can we wait with merging of this until the merge of #4239? That PR changes the approach to the config itself. Actually, after the 2.0 release there should NOT be breaking changes in the config and we should establish a clear deprecation and migration policy.

@BAStos525
Copy link
Contributor

From my side, it would be better to be notified via PR auto-assignment or just auto-comment with my username because it's better rather than to search for PRs with config-changed label. Smth like that:

permissions:
  pull-requests: write
jobs:
  api-changes:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: jitterbit/get-changed-files@v1
        continue-on-error: true
        id: api_label
      - uses: actions-ecosystem/action-add-labels@v1
        if: contains(steps.api_label.outputs.added_modified, 'docs/source/references/schema.json')
        with:
          github_token: ${{ secrets.github_token }}
          labels: |
            api-changes
      - uses: mshick/add-pr-comment@v2
        if: contains(steps.api_label.outputs.added_modified, 'docs/source/references/schema.json')
        with:
          message: @BAStos525 

But I understand that we should'n bind CI within any particular person.

@mversic mversic force-pushed the ci/config-changes_again branch from 2f6204a to 4c8c6e3 Compare February 17, 2024 05:37
@coveralls
Copy link

coveralls commented Feb 17, 2024

Pull Request Test Coverage Report for Build 7987011957

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2964 unchanged lines in 53 files lost coverage.
  • Overall coverage decreased (-0.3%) to 56.509%

Files with Coverage Reduction New Missed Lines %
core/src/sumeragi/network_topology.rs 1 98.78%
tools/kagami/src/crypto.rs 1 12.12%
data_model/src/metadata.rs 2 78.87%
primitives/src/unique_vec.rs 2 91.24%
config/src/kura.rs 3 80.0%
config/src/logger.rs 5 72.73%
logger/src/lib.rs 5 94.12%
tools/kagami/src/main.rs 5 0.0%
crypto/src/signature/secp256k1.rs 7 96.09%
telemetry/src/dev.rs 7 0.0%
Totals Coverage Status
Change from base Build 7884695009: -0.3%
Covered Lines: 22413
Relevant Lines: 39663

💛 - Coveralls

@s8sato
Copy link
Contributor Author

s8sato commented Feb 19, 2024

#4276 (comment)

Yeah, for now this PR is only intended to add clues to detect configuration changes.
Here are 2 other suggestions for making notifications actually work.
Do you think the following would work as well as your suggestion @BAStos525 ?

  1. Have a separate workflow dedicated to managing individual notifications for labeling, including manual labeling
  2. Have each person have web hooks to receive notifications

But before that, we have yet to see CI label config-changes in response to mock config changes in this PR

@BAStos525
Copy link
Contributor

#4276 (comment)

Yeah, for now this PR is only intended to add clues to detect configuration changes. Here are 2 other suggestions for making notifications actually work. Do you think the followings would work as well as your suggestion @BAStos525 ?

  1. Have a separate workflow dedicated to managing individual notifications for labeling, including manual labeling

  2. Have each person have web hooks to receive notifications

But before that, we have yet to see CI label config-changes in response to mock config changes in this PR

We will think about it later. I want say that we should try to avoid creating a new workflows for tiny tasks whatever it's possible.

@s8sato s8sato force-pushed the ci/config-changes_again branch from 4c8c6e3 to cd12942 Compare February 19, 2024 14:12
@s8sato s8sato changed the title [ci]: Give config-changes the role of an automatic label again [ci]: Fix auto-labeling Feb 19, 2024
Copy link
Contributor Author

@s8sato s8sato left a comment

Choose a reason for hiding this comment

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

This PR itself cannot trigger the changed workflows since pull_request_target event-driven workflows are based on the code of the base branch (hyperledger:iroha2-dev)
https://github.com/actions/labeler?tab=readme-ov-file#initial-set-up-of-the-labeler-action

.github/workflows/iroha2-dev-pr-title.yml Show resolved Hide resolved
.github/workflows/iroha2-label.yml Show resolved Hide resolved
@s8sato s8sato marked this pull request as ready for review February 19, 2024 14:45
@s8sato s8sato enabled auto-merge (squash) February 19, 2024 14:53
0x009922
0x009922 previously approved these changes Feb 20, 2024
.github/labeler.yml Outdated Show resolved Hide resolved
@s8sato s8sato disabled auto-merge February 20, 2024 08:07
BAStos525
BAStos525 previously approved these changes Feb 20, 2024
@s8sato s8sato dismissed stale reviews from BAStos525 and 0x009922 via b688b2f February 20, 2024 10:52
@s8sato s8sato force-pushed the ci/config-changes_again branch from cd12942 to b688b2f Compare February 20, 2024 10:52
@s8sato s8sato force-pushed the ci/config-changes_again branch from b688b2f to 7654a2f Compare February 20, 2024 11:07
.github/workflows/iroha2-dev-pr.yml Outdated Show resolved Hide resolved
.github/labeler.yml Show resolved Hide resolved
@s8sato s8sato force-pushed the ci/config-changes_again branch from 7654a2f to 7a511f5 Compare February 20, 2024 11:35
Signed-off-by: Shunkichi Sato <[email protected]>
@s8sato s8sato force-pushed the ci/config-changes_again branch from 7a511f5 to 6c21f53 Compare February 21, 2024 09:45
@s8sato s8sato merged commit effdf37 into hyperledger-iroha:iroha2-dev Feb 21, 2024
8 of 10 checks passed
@s8sato s8sato deleted the ci/config-changes_again branch February 22, 2024 05:04
@s8sato s8sato added the CI label Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants