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

Add execute --restart-request option #154

Merged
merged 4 commits into from
Jan 3, 2025
Merged

Add execute --restart-request option #154

merged 4 commits into from
Jan 3, 2025

Conversation

kkaarreell
Copy link
Collaborator

No description provided.

@kkaarreell
Copy link
Collaborator Author

I am not sure about the /regexp/ variant. I think it won't hurt if the passed string (e.g. REQ-1.2.1) would be always processed as regexp . Despite . can match any symbol, due to having digits in place and re.fullmatch() usage, in reality it should not match anything else... unless u user does a typo. What I like about using / is that it's explicit, in NEWA it is a new concept though.

@kkaarreell kkaarreell force-pushed the ks_restart_req branch 3 times, most recently from 55e6f57 to b0a9f0c Compare January 3, 2025 08:42
@The-Mule
Copy link
Collaborator

The-Mule commented Jan 3, 2025

Yeah, I am also not sure about RE. On one hand I like it, on the other hand I feel like it is not necessary. To me the idea of REQ re-execution is to be able to re-execute isolated REQs and since you can already use the option multiple times I don't really see much need for allowing RE (also, it feels like it would be easier to list the REQs explicitly rather than coming up with the right RE to match most of them). I appreciate the idea of distinguishing RE by /.../ but it needs a proper help description that is currently missing. All in all, It is a nice touch but I don't think it will be used much, for example this was what I had to do in my case:

newa --state-dir /var/tmp/newa/run-10 execute --restart-request '/REQ-4[38].124.4[38]/'

while the following variant is more readable and requires no RE implementation in this PR:

newa --state-dir /var/tmp/newa/run-10 execute --restart-request REQ-43.124.43 --restart-request REQ-48.124.48

Following the KISS principle I would slightly prefer "dumb" version without RE but I don't really mind it. I briefly tested it and it works fine. I think this is a really important addition that will be used a lot!

@kkaarreell
Copy link
Collaborator Author

OK, I have removed the regexp support. Btw, note the recently added short version -R.

@kkaarreell
Copy link
Collaborator Author

kkaarreell commented Jan 3, 2025

@The-Mule Would you mind reviewing&testing it one more time? Thank you.

@kkaarreell
Copy link
Collaborator Author

@The-Mule Would you mind reviewing&testing it one more time? Thank you.

testing feedback provided in #151 (comment)

@kkaarreell kkaarreell merged commit dbfffe1 into main Jan 3, 2025
13 checks passed
@kkaarreell kkaarreell deleted the ks_restart_req branch January 3, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants