-
Notifications
You must be signed in to change notification settings - Fork 21
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 backlog-set-due-date script #44
Add backlog-set-due-date script #44
Conversation
4a5232e
to
be7e32b
Compare
Example |
152874d
to
4963b4b
Compare
4963b4b
to
a9261c9
Compare
Thanks @okurz for the review. Following your ideas:
Right now the script is prepared for testing using the issues.json and doesn't uses the curl function The results of the tests are
|
Is this solution better now? If yes, I will clean up and start to implement the API update call |
Please see #44 (comment) first. I think you can simplify a lot for now and not use the switch on prio at all for now. Simple pseudo-code proposal:
|
Ok, let's do that |
Ok, I pushed a commit that has the shellcheck clean and should do the job. I didn't test this yet on real, but I tests the different parts using echos. If reviewers are ok, next step is to clean the commits and remove the test file to prepare to be merged. |
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 think the test file is a good idea to be used within an automatic integration test. At least please move the test file to "tests/". Then you can add a test for it. You mentioned "echo" already. This could be used for testing. I guess the easiest option is the same approach that we do for autoinst and openQA, e.g. a simple file like t/backlog-set-due-date.t
using Test::Most
. But it is ok for me if you see these automatic tests out of scope, just move the file then.
a1411b6
to
785c462
Compare
I moved out the testing file and reserved it to the testing PR. But for testing purposes should the echos be maintained in this script? and also a parameter to initialize the script from a file instead of perform the curl? and also avoid the final curl? |
Also I added a new commit with testing options |
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 tried it out now, two issues:
- Please phrase git commit message subject lines in imperative mood, i.e. "s/Added/Add/"
- This is setting the due date on too many tickets. E.g. "Blocked" ones, e.g. https://progress.opensuse.org/issues/69310 . This does not make sense because the assignee of a blocked ticket can't do anything about it, that's why it's "Blocked". We should exclude them or only select a passlist of status values where we want to set a due date. Also https://progress.opensuse.org/issues/80264 would have the due date set but that ticket does not have an assignee. My proposal had that mistake to look for
issue.assignee == null
when instead we want the opposite. And please keep in mind that I only wrote pseudo-code. I don't know if you an actually check for "null" in jq or anything
Working on that |
0314a1a
to
c52fc74
Compare
Ok, now the commit includes the reviews comments from @kalikiana and @okurz. Also include the dry mode with the a testing file to have predictable results |
c52fc74
to
043e2d0
Compare
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.
Please share some info on what you have tested after your recent changes. I see that you include "!=Blocked", does that exclude "Resolved" still?
No that doesn't include resolved. I can add that but I didn't thought was necessary. However adding that |
043e2d0
to
2df19e7
Compare
I added the Resolved exception too and changed the assignee by assigned_to. Let me know if there are more. I run the test
With the issues.json like this:
And the result output is
|
97018b2
to
a65b1fa
Compare
A new update with the suggestions of @kalikiana (from a chat) I also would like to share a test with real data in dry_mode (without updating the tickets)
|
a65b1fa
to
d28126a
Compare
This script sets the duedate to 14 days from the script's execution time for tickets that are not low priority and have not been assigned to anyone Add dry_run mode. It uses a test json file as first parameter to perform the tests and thus have predictable results progress.opensuse.org/issues/73468
d28126a
to
27973e4
Compare
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.
The script looks good. Can you add it to github actions now?
I have called it locally and it set the due date on three tickets
- https://progress.opensuse.org/issues/73468
- https://progress.opensuse.org/issues/80466
- https://progress.opensuse.org/issues/80492
all three tickets are correct. I have not checked if there should have been more tickets but that's ok.
Don't we should merge this before? |
In order to enable an action to the backlog-set-due-date script introduced in the PR os-autoinst#44 is needed this workflow
In order to enable an action to the backlog-set-due-date script introduced in the PR os-autoinst#44 is needed this workflow
In order to enable an action to the backlog-set-due-date script introduced in the PR os-autoinst#44 this PR modify the current workflow for WIP-limits to include also this script
This script sets the duedate to 14 days from the script's
execution time for tickets that are not low priority and
have not been assigned to anyone
Add dry_run mode. It uses a test json file as first parameter
to perform the tests and thus have predictable results
https://progress.opensuse.org/issues/73468