-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add a customization step to README instructions #35
Conversation
When running the QA potentially concurrently as part of a team, it's necessary to customize some infrastrucure configuration parameters to prevent collisions between instances set up for different users.
@@ -42,63 +42,70 @@ After you have all the prerequisites installed and have configured your | |||
EOF | |||
``` | |||
|
|||
4. Initialize Terraform (only needed once) | |||
4. If necessary, set the Makefile variables `DO_INSTANCE_TAGNAME` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is to be configured by experiment, I recommend it being done in experiment.mk instead of Makefile.
Other than that, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be as per #34.
It's a small distinction, as all variables can me overridden from the make
command line. Is experiment.mk
used in scripts somewhere so that it needs to be separate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. I broke it down so that that it would be easier to keep multiple versions of experiment.mk, for different experiments, without having to touch Makefile, which should remain untouched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I broke it down so that that it would be easier to keep multiple versions of experiment.mk, for different experiments, without having to touch Makefile, which should remain untouched.
Should the VERSION_TAG
/VERSION_WEIGHT
variables be moved there as well, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
An additional configuration step has been added to the README in the qa-infra repository (cometbft/qa-infra#35).
An additional configuration step has been added to the README in the qa-infra repository (cometbft/qa-infra#35).
The QA method document in the cometbft repository currently refers to numbered ranges of steps in this README as setup and run sequences that the tester needs to perform at different stages. When the list of steps changes as it did in #35, updating these hardcoded references is tedious and error-prone (case in point, I failed to update quite a few places). The solution is to do to these human-consumed texts what programmers do to machine code and introduce subroutines! The step sequences are split under headers that can be "invoked" by name and anchored in the HTML rendering.
* README: split steps between setup and test run The QA method document in the cometbft repository currently refers to numbered ranges of steps in this README as setup and run sequences that the tester needs to perform at different stages. When the list of steps changes as it did in #35, updating these hardcoded references is tedious and error-prone (case in point, I failed to update quite a few places). The solution is to do to these human-consumed texts what programmers do to machine code and introduce subroutines! The step sequences are split under headers that can be "invoked" by name and anchored in the HTML rendering.
When running the QA potentially concurrently as part of a team, it's necessary to customize some infrastrucure configuration parameters to prevent collisions between instances set up for different users.