Skip to content

Rewrite openqa-advanced-retrigger-jobs in python #332

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Rewrite openqa-advanced-retrigger-jobs in python #332

wants to merge 3 commits into from

Conversation

@okurz
Copy link
Member

@okurz okurz commented Jul 23, 2024

No description provided.

perlpunk

This comment was marked as resolved.

Sign in to view

@okurz

This comment was marked as resolved.

Sign in to view
aylen384
aylen384 reviewed Jul 23, 2024
View reviewed changes
@perlpunk

This comment was marked as resolved.

Sign in to view
@perlpunk
Copy link
Contributor

Also how about moving scripts to bin/ and adding an extension, .py. I don't have a problem with extensions, and it makes tooling like linting easier

@okurz
Copy link
Member Author

okurz commented Jul 23, 2024

Also how about moving scripts to bin/ and adding an extension, .py. I don't have a problem with extensions, and it makes tooling like linting easier

An extension would mean that we would need a separate installation tooling and step to install as executable without extension and I would like to avoid that.

@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch from f32c1d5 to f8d55f9 Compare July 24, 2024 07:00
@okurz
Copy link
Member Author

okurz commented Jul 24, 2024

It would still be good to have an example. I remember that it was complicated to get the quoting right

Done

nicksinger
nicksinger reviewed Jul 24, 2024
View reviewed changes
perlpunk
perlpunk reviewed Jul 24, 2024
View reviewed changes
perlpunk
perlpunk reviewed Jul 24, 2024
View reviewed changes
@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch from f8d55f9 to d00d067 Compare July 24, 2024 10:39
@mergify mergify bot mentioned this pull request Jul 24, 2024
Draft
@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch 2 times, most recently from bd97f6e to bdd1863 Compare July 25, 2024 18:12
@okurz
Copy link
Member Author

okurz commented Jul 25, 2024

Updated:

  • Simple and advanced example
  • Converted single string in subprocess.run with shell=True into list (or actually a set). For that I extracted the post function so that I can just print in case of "dry-run" instead of the "client prefix" which was previously used to "echo" instead in bash.

Tested manually with

while_inotifywait sh -c "python3.11 ./openqa-advanced-retrigger-jobs -vvvv --host openqa.suse.de --failed-since \"2024-07-23T13:00\" --result failed --additional-filters \"test not like '%:investigate:%' and id=14995768\" --comment \"my foo comment\" --dry-run && python3.11 ./openqa-advanced-retrigger-jobs -vvvv --host openqa.suse.de --failed-since \"2024-07-23T13:00\" --result failed --additional-filters \"test not like '%:investigate:%' and id=14995768\" --comment \"my foo comment\""

@okurz okurz removed the not-ready label Jul 25, 2024
Martchus
Martchus reviewed Jul 29, 2024
View reviewed changes
openqa-advanced-retrigger-jobs
Comment on lines +89 to +85
query = (
f"select id from jobs where ({worker_string}result='{args.result}' "
f"and clone_id is null and t_finished >= '{args.failed_since}'{additional_filters});"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally I'd say this needs escaping but of course the previous script also didn't have that…

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what additional escaping you mean. Why do you mean we need escaping?

Copy link
Member

Choose a reason for hiding this comment

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

I think he means something in the lines of https://www.psycopg.org/psycopg3/docs/basic/params.html#execute-arguments

Currently e.g. args.result could be used to insert arbitrary SQL statements

Copy link
Member

Choose a reason for hiding this comment

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

While malice is very unlikely (given you'd need access anyway) it could help if people accidentally pass arguments which cause the query to break

Copy link
Contributor

Choose a reason for hiding this comment

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

And one can also (accidentally) break out of the whole psql invocation.

@okurz okurz force-pushed the feature/openqa_advanced_retrigger_python2 branch from 9725eaf to 00ab178 Compare July 29, 2024 11:58
@mergify
Copy link
Contributor

mergify bot commented Mar 4, 2025

This pull request is now in conflicts. Could you fix it? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@perlpunk perlpunk perlpunk left review comments

@aylen384 aylen384 aylen384 left review comments

@Martchus Martchus Martchus left review comments

@nicksinger nicksinger nicksinger left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@okurz @perlpunk @aylen384 @Martchus @nicksinger