-
Notifications
You must be signed in to change notification settings - Fork 65
scripts: add an abstraction for the args that depend on each other and validate them in one place #9797
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
guyf-starkware
left a comment
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.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @ShahakShama)
scripts/prod/update_config_and_restart_nodes_lib.py line 600 at r1 (raw file):
for index, config in enumerate(configs): print(f"Applying config {index}...") # apply_configmap(
This is left commented out while in review to make sure we don't accidentally do something. Adding this blocking comment to avoid accidental merge with this still commented out
scripts/prod/update_config_and_restart_nodes_lib.py line 609 at r1 (raw file):
if restart_strategy != RestartStrategy.NO_RESTART: for index, config in enumerate(configs): # restart_pod(
This is left commented out while in review to make sure we don't accidentally do something. Adding this blocking comment to avoid accidental merge with this still commented out
35fa0cc to
34281ad
Compare
bb9e648 to
6828d34
Compare
34281ad to
578e5b4
Compare
6828d34 to
f1a3b8b
Compare
578e5b4 to
86d3ec4
Compare
86d3ec4 to
c180da2
Compare
f1a3b8b to
2fc24e1
Compare
c180da2 to
04be961
Compare
04be961 to
8277b17
Compare
ShahakShama
left a comment
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.
@ShahakShama reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @guyf-starkware)
scripts/prod/update_config_and_restart_nodes_lib.py line 600 at r1 (raw file):
Previously, guyf-starkware wrote…
This is left commented out while in review to make sure we don't accidentally do something. Adding this blocking comment to avoid accidental merge with this still commented out
Just making sure, you mean you'll uncomment this before merging (and not delete this), right?
scripts/prod/update_config_and_restart_nodes_lib.py line 609 at r1 (raw file):
Previously, guyf-starkware wrote…
This is left commented out while in review to make sure we don't accidentally do something. Adding this blocking comment to avoid accidental merge with this still commented out
Just making sure, you mean you'll uncomment this before merging (and not delete this), right?Just making sure, you mean you'll uncomment this before merging (and not delete this), right?
scripts/prod/update_config_and_restart_nodes_lib.py line 512 at r2 (raw file):
def get_cluster(self, index: int) -> Optional[str]: return self.cluster_list[index] if self.cluster_list else None
Our python conventions is to write explicitly if x is not None and not if x
…d validate them in one place
8277b17 to
b1f2a45
Compare
guyf-starkware
left a comment
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.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @ShahakShama)
scripts/prod/update_config_and_restart_nodes_lib.py line 600 at r1 (raw file):
Previously, ShahakShama wrote…
Just making sure, you mean you'll uncomment this before merging (and not delete this), right?
Yes. I made this blocking comment to make sure I don't mege by mistake before resolving the comment (and uncomment the code)
scripts/prod/update_config_and_restart_nodes_lib.py line 609 at r1 (raw file):
Previously, ShahakShama wrote…
Just making sure, you mean you'll uncomment this before merging (and not delete this), right?Just making sure, you mean you'll uncomment this before merging (and not delete this), right?
Same as above
scripts/prod/update_config_and_restart_nodes_lib.py line 512 at r2 (raw file):
Previously, ShahakShama wrote…
Our python conventions is to write explicitly
if x is not Noneand notif x
Done
b1f2a45 to
64f76e2
Compare
49d883c to
5e40bd6
Compare
guyf-starkware
left a comment
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.
@guyf-starkware reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @guyf-starkware)
guyf-starkware
left a comment
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.
@guyf-starkware reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @guyf-starkware)

No description provided.