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(cli): Always respect the --metrics-port disabled option #2459

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

matheus23
Copy link
Contributor

@matheus23 matheus23 commented Jul 4, 2024

Description

The --metrics-port disabled option that works with iroh start wasn't respected when running e.g. iroh --metrics-port disabled node status.

This caused CLI tests to fail when two or more commands were run concurrently due to port conflicts.

Breaking Changes

None

Notes & open questions

  1. Should we mark some tests as non-flaky?
    I think I'll try to run the tests that definitely work through CI a bit
  2. Does this need dedicated tests?
    It's... possible to test that, but technically the tests that were marked as flaky did cover this, but they weren't dedicated to the metric port option precisely. Thoughts?

Change checklist

  • Self-review.
  • [ ] Documentation updates following the style guide, if relevant. None needed really, just a fix.
  • [ ] Tests if relevant. IMO we already have tests covering this (they're just marked as flaky at the moment)
  • All breaking changes documented.

@matheus23 matheus23 self-assigned this Jul 4, 2024
@matheus23 matheus23 enabled auto-merge July 5, 2024 08:27
@matheus23 matheus23 added this pull request to the merge queue Jul 5, 2024
Merged via the queue into main with commit 2c40984 Jul 5, 2024
65 of 74 checks passed
@dignifiedquire dignifiedquire deleted the matheus23/cli-respect-metrics-opt branch July 5, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants