Skip to content

Conversation

@nwnt
Copy link
Member

@nwnt nwnt commented Oct 23, 2025

As discussed, I'm breaking #20817 down to something smaller. Hope this works, but please do let me know if you want to break it down further.

cc @serathius @ivanvc

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.22%. Comparing base (1f1750b) to head (a492abd).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

see 19 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #20844      +/-   ##
==========================================
+ Coverage   69.20%   69.22%   +0.01%     
==========================================
  Files         422      422              
  Lines       34822    34822              
==========================================
+ Hits        24100    24106       +6     
+ Misses       9334     9326       -8     
- Partials     1388     1390       +2     

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 1f1750b...a492abd. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cfg := common.MakeConfig(NodeCount)

hosts, reportPath, etcdetcdDataPaths := common.GetPaths(cfg)
if *local {
Copy link
Member

Choose a reason for hiding this comment

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

What about local?

@serathius
Copy link
Member

The title "Move to using paths from environment variables" is not reflecting what the PR. The PR does:

  • Removes local argument, but we use it in our documentation
  • Removes using defaults in environment variables are not passed

I addition you needlessly

  • Flip logic on if statements like if envEndpointsStr != "" to if envEndpointsStr == ""

Please send 3 PRs:

  1. Remove dependency on local
  2. Remove --local from documentation and from commands
  3. Flip logic.

@serathius
Copy link
Member

First PR #20845

@nwnt nwnt force-pushed the antithesis-use-env-paths-only branch from c5ce538 to 1e665ed Compare October 24, 2025 03:26
@nwnt
Copy link
Member Author

nwnt commented Oct 24, 2025

@serathius sorry I should not have sent this PR before going to bed last night. Indeed, I should have made the changes like #20845 first.

I have checked and I don't think we have any other places that refer to --local flag. The only places that use it happened to be where you already removed (i.e. in the makefile)

The next step is to flip the logic, which is what this PR does. I confirmed that I have done the following tests on the branch and they are all successful.

  • make antithesis-build-etcd-image
  • make antithesis-docker-compose-up
  • make antithesis-run-container-traffic
  • make antithesis-run-container-validation

The next step after this is to clean up CFG_NODECOUNT and GetPaths which is no longer used.

@serathius
Copy link
Member

Pushed commit with suggestions on top of your branch. Please let me know if this is simpler.

Signed-off-by: Marek Siarkowicz <[email protected]>
@serathius serathius force-pushed the antithesis-use-env-paths-only branch from ee277ef to a492abd Compare October 24, 2025 09:40
@nwnt nwnt requested a review from serathius October 24, 2025 14:08
@nwnt
Copy link
Member Author

nwnt commented Oct 24, 2025

That works and is simpler. I created that new function just to facilitate the change/diff, but it's no longer needed.

Thank you.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nwnt, 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 855c9ba into etcd-io:main Oct 24, 2025
33 checks passed
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.

3 participants