Skip to content

Enhancement: Small Improvement for Interactively Overriding Local Testing Harness Env Vars #429

Closed
tzaffi wants to merge 12 commits into
developfrom
temp-branch-improve-some-goal-errors
Closed

Enhancement: Small Improvement for Interactively Overriding Local Testing Harness Env Vars #429
tzaffi wants to merge 12 commits into
developfrom
temp-branch-improve-some-goal-errors

Conversation

@tzaffi
Copy link
Copy Markdown
Contributor

@tzaffi tzaffi commented Dec 30, 2022

Should I keep these changes around and merge harness_overwrite.py? This tinkering work grew out of an investigation of the downstream effects of this go-algorand PR: algorand/go-algorand#4951

Arguments For and Against Merging the PR

  • DO NOT MERGE
    • This violates the goal of keeping up.sh as similar as possible across all 4 SDK's
    • Even if we decide that this should exist in all 4 SDK's, it's written in Python, and we can't impose installing Python across the other SDK's
    • It's not useful enough to justify the future maintenance costs
  • MERGE
    • It's okay to have small "violations" of the uniformity goal, if there is a clear upside
    • Anything that eases the investigation of downstream implications for changes in go-algorand -even a little- is worthwhile
    • Python is so ubiquitous for shell scripting; we could impose requiring it in all the SDK's - imagine how much easier life would be if we could dispense with shell scripting in favor of python scripting?

Q: What does harness_overwrite.py do?

It makes it slightly easier locally to mod environments that the test harness needs. In particular

  1. set INTERACTIVE_TESTING_ENVIRONMENT=1 in .test-env
  2. run make harness and follow the prompts.

here are what the prompts look like

Which of the following env vars do you want to modify?

VERBOSE_HARNESS,TYPE,ALGOD_CHANNEL,ALGOD_URL,ALGOD_BRANCH,ALGOD_SHA,NETWORK_TEMPLATE,NETWORK_NUM_ROUNDS,INDEXER_URL,NODE_ARCHIVAL,INDEXER_BRANCH,INDEXER_SHA,SANDBOX_URL,SANDBOX_BRANCH,LOCAL_SANDBOX_DIR,ALGOD_CONTAINER,KMD_PORT,ALGOD_PORT,INDEXER_CONTAINER,INDEXER_PORT,POSTGRES_CONTAINER,POSTGRES_PORT

    (skip for ALL, or provide comma separated)
    
    A typical choice is:
TYPE,ALGOD_URL,ALGOD_BRANCH

CHOICES:

Then I entered the "typical choice" and proceeded with the next prompts:

...

CHOICES:
TYPE,ALGOD_URL,ALGOD_BRANCH
__________________________________________________
Please provide env variable overrides (skip to keep defaults)
TYPE (default `"channel"`):source
ALGOD_URL (default `"https://github.com/algorand/go-algorand"`):https://github.com/tzaffi/go-algorand
ALGOD_BRANCH (default `"master"`):improve-some-goal-errors
__________________________________________________
OVERWRITING
new_env[TYPE]=source (PREVIOUS: env[TYPE]="channel")
new_env[ALGOD_URL]=https://github.com/tzaffi/go-algorand (PREVIOUS: env[ALGOD_URL]="https://github.com/algorand/go-algorand")
new_env[ALGOD_BRANCH]=improve-some-goal-errors (PREVIOUS: env[ALGOD_BRANCH]="master")
__________________________________________________
...

Q: What are the implications for harness_overwrite.py in C.I.?

As long as one remembers not to merge in INTERACTIVE_TESTING_ENVIRONMENT=1 in .test-env,
C.I. will continue running as before. In order to run the same override environment in C.I., you
need to take the following steps:

  1. Follow the steps above to locally overwrite test-harness/.env
  2. Copy the file portion in the resulting .env file starting from TYPE=... all the way to the end into .test-env
  3. Make sure to set INTERACTIVE_TESTING_ENVIRONMENT=0 and OVERWRITE_TESTING_ENVIRONMENT=1
  4. Commit these changes and push to github

C.I. should now run against the new harness env variables.

Comment thread harness_overwrite.py Outdated
Comment thread test-harness.sh
Comment thread harness_overwrite.py
Comment thread harness_overwrite.py Outdated
@tzaffi tzaffi requested a review from bbroder-algo December 30, 2022 02:53
Comment thread harness_overwrite.py
return key, val, extra


def manualy_parse(env_file: str) -> Dict[str, str]:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"manually" as opposed with using dot_env which I couldn't figure out how to use to read from only one env file and ignore all other env variables.

@tzaffi tzaffi requested a review from algochoi December 30, 2022 13:17
@tzaffi tzaffi changed the title Temp-branch-improve-some-goal-errors Enhancement: Small Improvement for Interactively Overriding Local Testing Harness Env Vars Jan 1, 2023
@tzaffi tzaffi marked this pull request as ready for review January 1, 2023 15:27
Comment thread .test-env
Comment thread .test-env
# What can ease some of the pain above is to overwrite interactively:
INTERACTIVE_TESTING_ENVIRONMENT=0


Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  • THE FOLLOWING SHOULD BE DELETED BEFORE MERGING

Comment thread .test-env Outdated
@bbroder-algo
Copy link
Copy Markdown
Contributor

zeph how do we feel about this one?

@tzaffi
Copy link
Copy Markdown
Contributor Author

tzaffi commented Feb 15, 2023

zeph how do we feel about this one?

I think it's kind of cool, but someone else should give a spin to make sure it's worthwhile. I list some pros/cons above, the big con being the future maintenance cost.

@algochoi
Copy link
Copy Markdown
Contributor

I think it is a nice improvement for local testing, and could be worthwhile having it in a long lasting fork or a branch, and reference it in the docs.

I don't feel as strongly about merging this into develop though, for reasons Zeph outlined above. I think having a uniform CI process across all the SDKs seems favorable. If this was implemented/enforced across the four SDKs we manage, I'd be more inclined to merge it in.

@tzaffi
Copy link
Copy Markdown
Contributor Author

tzaffi commented Feb 16, 2023

I think it is a nice improvement for local testing, and could be worthwhile having it in a long lasting fork or a branch, and reference it in the docs.

I don't feel as strongly about merging this into develop though, for reasons Zeph outlined above. I think having a uniform CI process across all the SDKs seems favorable. If this was implemented/enforced across the four SDKs we manage, I'd be more inclined to merge it in.

I'm not inclined to pursue a strong counter-argument recognizing that much of my excitement is due to my personal investment in it. Unless I hear back from more people by the end of the week, I'll close this PR (but will keep a copy in my personal repo for future testing needs).

@tzaffi
Copy link
Copy Markdown
Contributor Author

tzaffi commented Feb 17, 2023

I'm closing this PR as there was insufficient interest in pursuing it further. However, I'm keeping a copy on my personal repo for future reference.

@tzaffi tzaffi closed this Feb 17, 2023
@tzaffi tzaffi deleted the temp-branch-improve-some-goal-errors branch February 17, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants