Skip to content

fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls #2295

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.

Sign up for GitHub

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

Merged
onur-ozkan merged 6 commits into from
Dec 25, 2024
Merged

fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls #2295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0576c25
add feature flag to mm2_main and mm2_bin_lib tomls
laruh Dec 16, 2024
86f8312
filter 'status:' pr labels
laruh Dec 16, 2024
201bb32
remove old labels
laruh Dec 16, 2024
07b2f85
dont allow to pass blocked and invalid statuses
laruh Dec 16, 2024
456eb17
pass in progress and pending review statuses
laruh Dec 16, 2024
3678a8b
update statuses list
laruh Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/workflows/pr-lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ jobs:
- name: Check PR labels
env:
LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }}
if: contains(env.LABEL_NAMES, 'under review') == contains(env.LABEL_NAMES, 'in progress')
if: "!((contains(env.LABEL_NAMES, 'pending review') && !contains(env.LABEL_NAMES, 'in progress') && !contains(env.LABEL_NAMES, 'blocked'))
Copy link
Copy Markdown
Collaborator

@dimxy dimxy Dec 16, 2024

Choose a reason for hiding this comment

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

maybe you could use '!=' as 'xor' ?

if: ! (contains(env.LABEL_NAMES, 'pending review') != contains(env.LABEL_NAMES, 'in progress') != 
   contains(env.LABEL_NAMES, 'blocked'))
   ...
   exit 1

Copy link
Copy Markdown
Collaborator

@mariocynicys mariocynicys Dec 16, 2024

Choose a reason for hiding this comment

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

xor would treat true, true, true the same as true, false, false. that said im not sure how an expression with multiple == or != execute (they always confuse me so i just pretend they never existed xD) so you are using xor here as an analogy or they actually have the very same effect.
solutions i could think of:

Copy link
Copy Markdown
Collaborator

@dimxy dimxy Dec 16, 2024

Choose a reason for hiding this comment

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

Yes 'xor' would not work. Looks like there is no '+' operator either.
I guess we need to set env vars to sum them (like it was done in one of the recent commits)

Copy link
Copy Markdown
Collaborator

@mariocynicys mariocynicys Dec 16, 2024

Choose a reason for hiding this comment

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

oh. indeed + isn't listed in the op list.

maybe we can just avoid github actions' if clause and write the complete logic inside the run: in good old bash instead.

Copy link
Copy Markdown
Author

@laruh laruh Dec 17, 2024 •
edited by mariocynicys
Loading

Choose a reason for hiding this comment

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

@dimxy @mariocynicys

I suggest to move to bash if we decide to make lint logic more complex.
bash is more flexible, but less readable with more code lines required.

If we all agree with current labels logic:

Allow any number of labels, but only one of the following labels must be present at a time: ['status: pending review', 'status: in progress', 'status: blocked']. These labels cannot be set together.

then I suggest to leave GH actions expression.

Copy link
Copy Markdown
Author

@laruh laruh Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

bash is more flexible, but less readable with more code lines required.

i would argue that the current check is harder to read than if it was written in bash.

it would look like this

- name: Check PR labels
  env:
    LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }}
  run: |
    # Convert LABEL_NAMES (JSON) to a string
    LABEL_NAMES_STR=$(echo "${LABEL_NAMES}" | jq -r '.[]')

    # Define allowed labels
    ALLOWED_LABELS=("status: pending review" "status: in progress" "status: blocked")
    MATCHED_COUNT=0

    # Check for matches in LABEL_NAMES
    for label in "${ALLOWED_LABELS[@]}"; do
      if echo "$LABEL_NAMES_STR" | grep -Fxq "$label"; then
        ((MATCHED_COUNT++))
      fi
    done

    # Validate exactly one match
    if [[ "$MATCHED_COUNT" -ne 1 ]]; then
      echo "PR must have exactly one of these labels: ['status: pending review', 'status: in progress', 'status: blocked']."
      exit 1
    fi

this is too much, github actions allow to do the same in a more light weight way right in if block without waisting execution time on run

if we need to just be sure that labels have only one of 3 necessary labels this is simpler

      - name: Check PR labels
        env:
          LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }}
        if: "!((contains(env.LABEL_NAMES, 'pending review') && !contains(env.LABEL_NAMES, 'in progress') && !contains(env.LABEL_NAMES, 'blocked'))
          || (!contains(env.LABEL_NAMES, 'pending review') && contains(env.LABEL_NAMES, 'in progress') && !contains(env.LABEL_NAMES, 'blocked'))
          || (!contains(env.LABEL_NAMES, 'pending review') && !contains(env.LABEL_NAMES, 'in progress') && contains(env.LABEL_NAMES, 'blocked')))"
        run: |
          echo "PR must have "exactly one" of these labels: ['status: pending review', 'status: in progress', 'status: blocked']."
          exit 1

Copy link
Copy Markdown
Collaborator

@dimxy dimxy Dec 17, 2024

Choose a reason for hiding this comment

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

Maybe we can do 2 separate checks:
we have only one PR label starting with "status:" (if not then error 'duplicate status')
PR labels contain any from the list of allowed labels.

Copy link
Copy Markdown
Author

@laruh laruh Dec 18, 2024

Choose a reason for hiding this comment

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

we have only one PR label starting with "status:" (if not then error 'duplicate status')

I dont see problem of having status: pending review and status: pending contract deployment or status: pending infra etc together.
Only one status i doubt is status: invalid I would move it to PR must have "exactly one" of these labels list

PR labels contain any from the list of allowed labels.

I dont understand what do you mean, above you suggested to have only one status label

Copy link
Copy Markdown
Collaborator

@dimxy dimxy Dec 18, 2024

Choose a reason for hiding this comment

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

I dont understand what do you mean, above you suggested to have only one status label

I mean if we have already ensured that the PR has only single "status:" label,
it is easy to check that this label is one from allowed, like: contains(env.LABEL_NAMES, 'pending review') || contains(env.LABEL_NAMES, 'in progress') || contains(env.LABEL_NAMES, 'blocked')

Copy link
Copy Markdown
Author

@laruh laruh Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I dont understand what do you mean, above you suggested to have only one status label

I mean if we have already ensured that the PR has only single "status:" label, it is easy to check that this label is one from allowed, like: contains(env.LABEL_NAMES, 'pending review') || contains(env.LABEL_NAMES, 'in progress') || contains(env.LABEL_NAMES, 'blocked')

these also will lead to move the whole logic to run block using bash, while we can do the same right in one if block using GH actions expressions and it will use run only in the case of false result.

      - name: Determine label validity
        id: label_check
        env:
          LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }}
        run: |
          status_count=$(echo "$LABEL_NAMES" | jq '[.[] | select(startswith("status:"))] | length')

          allowed_status_count=$(echo "$LABEL_NAMES" | jq '[.[] | select(. == "status: pending review" or . == "status: in progress" or . == "status: blocked")] | length')

          if [ "$status_count" -eq 1 ] && [ "$allowed_status_count" -eq 1 ]; then
            # Valid scenario
            echo "valid=true" >> $GITHUB_OUTPUT
          else
            # Invalid scenario
            echo "valid=false" >> $GITHUB_OUTPUT
          fi

      - name: Check PR labels
        if: steps.label_check.outputs.valid == 'false'
        run: |
          echo "PR must have exactly one of these labels: ['status: pending review', 'status: in progress', 'status: blocked']."
          exit 1

|| (!contains(env.LABEL_NAMES, 'pending review') && contains(env.LABEL_NAMES, 'in progress') && !contains(env.LABEL_NAMES, 'blocked'))
|| (!contains(env.LABEL_NAMES, 'pending review') && !contains(env.LABEL_NAMES, 'in progress') && contains(env.LABEL_NAMES, 'blocked')))"
run: |
echo "PR must have "exactly one" of these labels: ['under review', 'in progress']."
echo "PR must have "exactly one" of these labels: ['status: pending review', 'status: in progress', 'status: blocked']."
exit 1
1 change: 1 addition & 0 deletions mm2src/mm2_bin_lib/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ custom-swap-locktime = ["mm2_main/custom-swap-locktime"] # only for testing purp
native = ["mm2_main/native"] # Deprecated
track-ctx-pointer = ["mm2_main/track-ctx-pointer"]
zhtlc-native-tests = ["mm2_main/zhtlc-native-tests"]
test-ext-api = ["mm2_main/test-ext-api"]

[[bin]]
name = "mm2"
Expand Down
1 change: 1 addition & 0 deletions mm2src/mm2_main/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ run-device-tests = []
enable-sia = ["coins/enable-sia", "coins_activation/enable-sia"]
sepolia-maker-swap-v2-tests = []
sepolia-taker-swap-v2-tests = []
test-ext-api = ["trading_api/test-ext-api"]

[dependencies]
async-std = { version = "1.5", features = ["unstable"] }
Expand Down